Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1156473007:
Enables the user to select multiple languages for spellchecking (UI) (Closed)
|
Created:
5 years, 6 months ago by Julius Modified:
5 years, 5 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, michaelpg+watch-options_chromium.org, yusukes+watch_chromium.org, shuchen+watch_chromium.org, rouslan+spellwatch_chromium.org, nona+watch_chromium.org, rlp+watch_chromium.org, arv+watch_chromium.org, asvitkine+watch_chromium.org, groby+spellwatch_chromium.org, Dan Beam, Ilya Sherman, groby-ooo-7-16, Avi (use Gerrit) 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.
TEST=*SpellcheckService*
TEST=*Multilanguage*
Original patch from Klemen Forstnerič (klemen.forstneric@gmail.com).
BUG=5102
Committed: https://crrev.com/963f5dcf387298e03aeb7c3654890d1ca28d2ca6
Cr-Commit-Position: refs/heads/master@{#338174}
Patch Set 1 #
Total comments: 14
Patch Set 2 : Replied to comments, added browser tests, rebased. #
Total comments: 38
Patch Set 3 : Addressed comments, clarified code. #
Total comments: 53
Patch Set 4 : Addressed previous comments. #
Total comments: 50
Patch Set 5 : Addressed comments, added browsertest, fixed failures. #
Total comments: 60
Patch Set 6 : Refactored some tests and addressed comments. #
Total comments: 26
Patch Set 7 : Addressed comments, removed unused includes. #
Total comments: 4
Patch Set 8 : Fixed nits. #
Total comments: 2
Patch Set 9 : Fixed a misleading comment. #
Total comments: 70
Patch Set 10 : Moved spellcheck_service_browsertest to a unittest, cleaned code, replied to comments, stopped Mac … #
Total comments: 2
Patch Set 11 : Disabled service client when multilingual spellcheck is enabled. #
Total comments: 2
Patch Set 12 : Fixed some bugs and handled the spelling service. #
Total comments: 32
Patch Set 13 : Fixed nits and refactored a test. #Patch Set 14 : Reverted from using any #if defines to exclude Mac compiles. #
Total comments: 12
Patch Set 15 : Fixed bug and added a test for it. #
Total comments: 6
Patch Set 16 : Fixed nits and rebased. #
Total comments: 56
Patch Set 17 : Replied to old comments and fixed small issues. #
Total comments: 40
Patch Set 18 : Answered comments. #
Total comments: 22
Patch Set 19 : Removed some unused includes. #Patch Set 20 : Fixed nits. #
Total comments: 11
Patch Set 21 : Fixed some clarity issues. #
Total comments: 18
Patch Set 22 : Renamed the browsertests and cleaned some things up. #
Total comments: 18
Patch Set 23 : Fixed nits and presubmit warnings. #Messages
Total messages: 124 (59 generated)
juliusa@google.com changed reviewers: + avi@chromium.org, dbeam@chromium.org, groby@chromium.org, isherman@chromium.org, rouslan@chromium.org
Replied to previous comments and added browser_tests. Picked up where Klemen Forstneric (klemen.forstneric@gmail.com) left off. https://codereview.chromium.org/1156473007/diff/1/chrome/browser/renderer_con... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/1156473007/diff/1/chrome/browser/renderer_con... 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 Moved into the if-statement where it's used. https://codereview.chromium.org/1156473007/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/language_options.html (right): https://codereview.chromium.org/1156473007/diff/1/chrome/browser/resources/op... 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) Seems like this long id is the way to go. I did, however, delete the '-' in "spell-check" for consistency's sake (in all other code we use "spellcheck" as one word). https://codereview.chromium.org/1156473007/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/language_options.js:65: * @type {string} >nit: @const {string} is shorter (and a newer syntax) Done. (and changed where it appears elsewhere) https://codereview.chromium.org/1156473007/diff/1/chrome/browser/resources/op... 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}. Done. https://codereview.chromium.org/1156473007/diff/1/chrome/browser/resources/op... 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_ Done. https://codereview.chromium.org/1156473007/diff/1/chrome/browser/resources/op... 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) spellCheckDictionary_ is a string representing the language that is being used. Similarly, spellCheckDictionaries_ is a dictionary of string:bool pairs for every language being used. These have been renamed to spellCheckLanguage_ and spellCheckLanguages_ respectively. https://codereview.chromium.org/1156473007/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/language_options.js:688: >var status = this.spellcheckDictionaryDownloadStatus_[languageCode]; >// ... re-use status ... > >or better yet, use a `switch () {` here Done. https://codereview.chromium.org/1156473007/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/spellcheck_factory.cc (right): https://codereview.chromium.org/1156473007/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_factory.cc:76: switches::kEnableMultilingualSpellChecker)) { >How is preference migration handled? (I.e. what happens if I enable/disable >multiple spellchecks) This should suffice for now but a TODO has been added to implement migration. https://codereview.chromium.org/1156473007/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_factory.cc:80: user_prefs::PrefRegistrySyncable::UNSYNCABLE_PREF); >do these preferences need to be migrated? This should suffice for now but a TODO has been added to implement migration. https://codereview.chromium.org/1156473007/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/1156473007/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_service.cc:113: *languages = dictionary_languages; >Can you please leave a short comment somewhere how the function decides which >languages to use? Done. https://codereview.chromium.org/1156473007/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_service.cc:118: std::set<int> selected_language_indices; >If it's always the first <n> elements, why not return <n> instead of a >set/vector? Done. https://codereview.chromium.org/1156473007/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/spellcheck_service.h (right): https://codereview.chromium.org/1156473007/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_service.h:62: // returns the set of indices of the current spell check languages in the >nit: can you change "in the vector" to "in |languages|"? Done. https://codereview.chromium.org/1156473007/diff/1/chrome/browser/spellchecker... chrome/browser/spellchecker/spellcheck_service.h:66: static std::set<int> GetSpellCheckLanguages( >Is there a particular reason to make it a set? Would vector do? Changed to an int because this return value is supposed to be the indices of the currently selected spellcheck languages with are always the first elements of |languages|. https://codereview.chromium.org/1156473007/diff/1/chrome/browser/spellchecker... File chrome/browser/spellchecker/spellcheck_service_unittest.cc (right): https://codereview.chromium.org/1156473007/diff/1/chrome/browser/spellchecker... 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 :) GetSpellCheckLanguagesFromAcceptedLanguages is void. But browser tests for GetSpellCheckLanguages were added to spellcheck_service_browsertests.cc
juliusa@google.com changed reviewers: - avi@chromium.org, dbeam@chromium.org, groby@chromium.org, isherman@chromium.org
Please hold for reviewing, still getting used to code review tool.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by juliusa@google.com
The CQ bit was unchecked by juliusa@google.com
Patchset #2 (id:60001) has been deleted
Patchset #2 (id:80001) has been deleted
Replied to the previous comments and added a couple browser tests.
A few initial comments to keep you busy. Let me know if you have questions. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/about_f... chrome/browser/about_flags.cc:2277: Remove this line to preserve the conventions of the rest of this file. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/about_f... chrome/browser/about_flags.cc:2400: Remove this line to preserve the conventions of the rest of this file. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/about_f... chrome/browser/about_flags.cc:2420: Remove this line to preserve the conventions of the rest of this file. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/about_f... chrome/browser/about_flags.cc:2430: #endif // defined(ENABLE_SPELLCHECK) && (defined(OS_LINUX) || defined(OS_WIN) Remove the " // defined..." after #endif to preserve the conventions of the rest of the file. This #endif is so close to the corresponding #if that its purpose is already clear enough. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h (right): https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h:8: #include <set> Remove unused include. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h:47: int selected_languages_; Add a comment on what this variable is. Change it's type to "size_t", because it represents a size of a vector. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (left): https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:34: language_selected_(0) { Don't remove initialization of a member variable. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:63: if (command_line->HasSwitch(switches::kEnableMultilingualSpellChecker)) { Please put "command_line->HasSwitch(switches::kEnableMultilingualSpellChecker))" into some function in spellcheck_common.h and use that function here instead. That eases a gradual rollout of this feature to our users. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.cc (left): https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:145: Don't change formatting here. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:97: std::vector<std::string>* languages) { Do not increase indent here. Use https://code.google.com/p/chromium/wiki/ClangFormat https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:99: std::vector<std::string> dictionary_languages = Avoid too many std::vector<std::string> copies by writing chrome::spellcheck_common::GetDictionaryLanguagesPref(prefs) directly to |languages| below. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:102: std::vector<std::string> accept_languages; Place |accept_languages| closer to where it's used. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:116: // followed by all other |accept_languages| not in |dictionary_languages|. This information is important. Put it in the header file. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:122: // many of the first elements in |languages| are dictionary languages. This information is important. Put it in the header file. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:141: Don't change formatting here. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.h (right): https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.h:69: // |languages|. s/current/enabled Explain why the return value is useful and sufficient. Why is std::set<> or std::vector<> of indices into |languages| not returned here? (The source file has a comment that explain this. That comment should be moved here.) https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.h:72: static int GetSpellCheckLanguages( Since you're returning the number of the first |languages| items that are enabled spellcheck languages, you should change the return type to "size_t". No need to change formatting. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.h:73: content::BrowserContext* context, Can you make this pointer "const" to clarify that this is a read-only function? https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.h:78: static void GetSpellCheckLanguagesFromAcceptLanguages( Inline this function's code into GetSpellCheckLanguages. It's not used anywhere else.
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
Addressed previous comments, used newly created MultilingualSpellcheckIsEnabled() function where needed. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/about_f... chrome/browser/about_flags.cc:2277: On 2015/06/04 19:41:05, Rouslan wrote: > Remove this line to preserve the conventions of the rest of this file. Done. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/about_f... chrome/browser/about_flags.cc:2400: On 2015/06/04 19:41:05, Rouslan wrote: > Remove this line to preserve the conventions of the rest of this file. Done. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/about_f... chrome/browser/about_flags.cc:2420: On 2015/06/04 19:41:05, Rouslan wrote: > Remove this line to preserve the conventions of the rest of this file. Done. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/about_f... chrome/browser/about_flags.cc:2430: #endif // defined(ENABLE_SPELLCHECK) && (defined(OS_LINUX) || defined(OS_WIN) On 2015/06/04 19:41:05, Rouslan wrote: > Remove the " // defined..." after #endif to preserve the conventions of the > rest of the file. This #endif is so close to the corresponding #if that its > purpose is already clear enough. Done. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h (right): https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h:8: #include <set> On 2015/06/04 19:41:05, Rouslan wrote: > Remove unused include. Done. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h:47: int selected_languages_; On 2015/06/04 19:41:05, Rouslan wrote: > Add a comment on what this variable is. Change it's type to "size_t", because it > represents a size of a vector. Done. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (left): https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:34: language_selected_(0) { On 2015/06/04 19:41:05, Rouslan wrote: > Don't remove initialization of a member variable. Done. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:63: if (command_line->HasSwitch(switches::kEnableMultilingualSpellChecker)) { On 2015/06/04 19:41:05, Rouslan wrote: > Please put "command_line->HasSwitch(switches::kEnableMultilingualSpellChecker))" > into some function in spellcheck_common.h and use that function here instead. > That eases a gradual rollout of this feature to our users. Done. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.cc (left): https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:145: On 2015/06/04 19:41:05, Rouslan wrote: > Don't change formatting here. Done. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:97: std::vector<std::string>* languages) { On 2015/06/04 19:41:05, Rouslan wrote: > Do not increase indent here. Use > https://code.google.com/p/chromium/wiki/ClangFormat Done. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:99: std::vector<std::string> dictionary_languages = On 2015/06/04 19:41:05, Rouslan wrote: > Avoid too many std::vector<std::string> copies by writing > chrome::spellcheck_common::GetDictionaryLanguagesPref(prefs) directly to > |languages| below. Done. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:102: std::vector<std::string> accept_languages; On 2015/06/04 19:41:05, Rouslan wrote: > Place |accept_languages| closer to where it's used. Done. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:116: // followed by all other |accept_languages| not in |dictionary_languages|. On 2015/06/04 19:41:05, Rouslan wrote: > This information is important. Put it in the header file. Done. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:122: // many of the first elements in |languages| are dictionary languages. On 2015/06/04 19:41:05, Rouslan wrote: > This information is important. Put it in the header file. Done. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:141: On 2015/06/04 19:41:05, Rouslan wrote: > Don't change formatting here. Done. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.h (right): https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.h:69: // |languages|. On 2015/06/04 19:41:06, Rouslan wrote: > s/current/enabled > > Explain why the return value is useful and sufficient. Why is std::set<> or > std::vector<> of indices into |languages| not returned here? (The source file > has a comment that explain this. That comment should be moved here.) Done. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.h:72: static int GetSpellCheckLanguages( On 2015/06/04 19:41:05, Rouslan wrote: > Since you're returning the number of the first |languages| items that are > enabled spellcheck languages, you should change the return type to "size_t". > > No need to change formatting. Done. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.h:73: content::BrowserContext* context, On 2015/06/04 19:41:06, Rouslan wrote: > Can you make this pointer "const" to clarify that this is a read-only function? Done. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.h:78: static void GetSpellCheckLanguagesFromAcceptLanguages( On 2015/06/04 19:41:06, Rouslan wrote: > Inline this function's code into GetSpellCheckLanguages. It's not used anywhere > else. Done.
Awesome progress so far! I don't want to bog you down with too many comments, so here's a few. I will stop reviewing until I see a response to these, OK? https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:54: SpellcheckService::GetSpellCheckLanguages(browser_context, &languages_); TODO(ROUSLAN): VERIFY THAT languages_ IS CLEARED OR IS ALREADY CLEAR IN GetSpellCheckLanguages(). https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:58: const base::CommandLine* command_line = You can move this statement down to where it was originally. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:131: (size_t)(command_id - IDC_SPELLCHECK_LANGUAGES_FIRST); static_cast<size_t>(command_id - IDC_SPELLCHECK_LANGUAGES_FIRST) https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resourc... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:26: * @const {string} Unless some Chrome tool or bot told you to do it this way, it's best to not modify notations here. There's a server that periodically runs Closure compiler (I think) on these files and will bug you if you're loosey goosey with your comment conventions. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:60: * @const {string} Best to following existing conventions of this file. @type {string} @const https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:145: spellCheckLanguage_: '', I thinks this renaming is not necessary and makes the code more confusing. Can you undo it? (You can keep the rename if you can explain/justify it.) https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:153: spellCheckLanguages_: {}, If you undo rename of spellCheckDictionary_, then this var should be named spellCheckDictionaries_. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:153: spellCheckLanguages_: {}, Can you look through Klemen's review and see whether he had a good justification for using a dictionary instead of a list here? https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:984: var languageCode = e.value.value; Inline this variable into the next statement. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:986: if (!cr.isMac) { Make both handleSpellCheckDictionaryPrefChange_() and handleSpellCheckDictionariesPrefChange_() do either one of "if (!cr.isMac) { do work }" or "if (cr.isMac) return". The difference is jarring. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:995: * @param {Event} e. Preference change event where e.value.value is the No period (.) after the first mention of the variable name |e|. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:1003: var commaSeparatedLanguageCodes = e.value.value; Inline this variable into the next statement. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:1007: for (var i = 0; i < languageCodesSplit.length; i++) Easier to write this? for (var i in languageCodesSplit) this.spellCheckLanguages_[languageCodesSplit[i]] = true; https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:1058: if (this.spellCheckLanguages_.hasOwnProperty(currentLanguageCode)) Are you able to iterate over the elements in this.spellCheckLanguages_ instead? https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.cc (left): https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:132: languages->push_back(dictionary_language); That's a lovely git diff. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:99: user_prefs::UserPrefs::Get(const_cast<content::BrowserContext*>(context)); Eh, const_cast like this is bad. Let's remove it and keep |context| non-const. Sorry, I should've checked the user_prefs::UserPrefs::Get() API before my previous recommendation. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:115: size_t dictionary_size = languages->size(); s/dictionary_size/enabled_spellcheck_languages Does this name make it more clear what's going on? https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.h (right): https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.h:66: // This function computes a vector of strings, |languages|, which are to be // Computes |languages| to display in the context menu... https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.h:67: // displayed in the context menu over a text area for changing spell check s/spell check/spellcheck Stick with your own conventions :-) https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.h:68: // languages. It returns the number of enabled spellcheck languages in Not sure how to say this better, but let me try to shorten it: // Computes |languages| to display in the context menu over a text area for changing spellcheck languages. Returns the number of the first spellcheck |languages| that are enabled. (Alt-q in Sublime to reformat the comment paragraph. Lines should be at most 80 char long.) https://codereview.chromium.org/1156473007/diff/160001/chrome/common/spellche... File chrome/common/spellcheck_common.cc (right): https://codereview.chromium.org/1156473007/diff/160001/chrome/common/spellche... chrome/common/spellcheck_common.cc:202: auto empty_strings_begin = Inline this variable into the next statement. Don't worry about formatting. Clang-format is your friend. :-) https://codereview.chromium.org/1156473007/diff/160001/chrome/common/spellche... chrome/common/spellcheck_common.cc:214: const base::CommandLine* command_line = Inline this variable into the next statement. Again, use clang-format afterward. https://codereview.chromium.org/1156473007/diff/160001/chrome/common/spellche... File chrome/common/spellcheck_common.h (right): https://codereview.chromium.org/1156473007/diff/160001/chrome/common/spellche... chrome/common/spellcheck_common.h:63: // Reads the spellcheck.dictionary or spellcheck.dictionaries preference No need to specify so much implementation details, especially since "spellcheck.dictionary" preference will eventually go away. // Returns the list of enabled spellcheck dictionaries. https://codereview.chromium.org/1156473007/diff/160001/chrome/common/spellche... chrome/common/spellcheck_common.h:68: // Checks if the command-line switch enable-multilingual-spellchecker is set. This comment will soon grow stale, because command-line will be only one of several ways to enable multilingual spellchecker. // Returns true if multilingual spellchecker is enabled. https://codereview.chromium.org/1156473007/diff/160001/chrome/common/spellche... chrome/common/spellcheck_common.h:69: bool MultilingualSpellcheckIsEnabled(); Newline after.
https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:54: SpellcheckService::GetSpellCheckLanguages(browser_context, &languages_); On 2015/06/05 17:50:04, Rouslan wrote: > TODO(ROUSLAN): VERIFY THAT languages_ IS CLEARED OR IS ALREADY CLEAR IN > GetSpellCheckLanguages(). Yep, it's cleared. Move along, nothing to see here.
Addressed previous comments. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:54: SpellcheckService::GetSpellCheckLanguages(browser_context, &languages_); On 2015/06/05 17:51:15, Rouslan wrote: > On 2015/06/05 17:50:04, Rouslan wrote: > > TODO(ROUSLAN): VERIFY THAT languages_ IS CLEARED OR IS ALREADY CLEAR IN > > GetSpellCheckLanguages(). > > Yep, it's cleared. Move along, nothing to see here. Acknowledged. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:54: SpellcheckService::GetSpellCheckLanguages(browser_context, &languages_); On 2015/06/05 17:50:04, Rouslan wrote: > TODO(ROUSLAN): VERIFY THAT languages_ IS CLEARED OR IS ALREADY CLEAR IN > GetSpellCheckLanguages(). Acknowledged. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:58: const base::CommandLine* command_line = On 2015/06/05 17:50:04, Rouslan wrote: > You can move this statement down to where it was originally. Done. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:131: (size_t)(command_id - IDC_SPELLCHECK_LANGUAGES_FIRST); On 2015/06/05 17:50:04, Rouslan wrote: > static_cast<size_t>(command_id - IDC_SPELLCHECK_LANGUAGES_FIRST) Done. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resourc... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:26: * @const {string} On 2015/06/05 17:50:05, Rouslan wrote: > Unless some Chrome tool or bot told you to do it this way, it's best to not > modify notations here. There's a server that periodically runs Closure compiler > (I think) on these files and will bug you if you're loosey goosey with your > comment conventions. I changed this because a comment on Klemen's patch mentioned to change it so I just changed it wherever it showed up. I guess this is the newer way of doing it (https://developers.google.com/closure/compiler/docs/js-for-compiler )? I could change other similar comment styles in the file to match it or just change these all back. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:60: * @const {string} On 2015/06/05 17:50:05, Rouslan wrote: > Best to following existing conventions of this file. > > @type {string} > @const See previous comment. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:145: spellCheckLanguage_: '', On 2015/06/05 17:50:05, Rouslan wrote: > I thinks this renaming is not necessary and makes the code more confusing. Can > you undo it? (You can keep the rename if you can explain/justify it.) I changed it because in Klemen's patch someone mentioned some confusion about spellCheckDictionary being a dictionary. I'll change it back though because looking through all other code it's known that the "dictionary" refers to the language name, and not an actual data structure comprising the words in the language. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:153: spellCheckLanguages_: {}, On 2015/06/05 17:50:05, Rouslan wrote: > If you undo rename of spellCheckDictionary_, then this var should be named > spellCheckDictionaries_. Done. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:153: spellCheckLanguages_: {}, On 2015/06/05 17:50:05, Rouslan wrote: > Can you look through Klemen's review and see whether he had a good justification > for using a dictionary instead of a list here? Dan Beam had a write-up about it here ( https://codereview.chromium.org/654653002/diff/400001/chrome/browser/resource... ). Initially I thought a list might be better too, but I think since we don't care about an order and it's faster to check if something is in an object this is the way to go. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:984: var languageCode = e.value.value; On 2015/06/05 17:50:05, Rouslan wrote: > Inline this variable into the next statement. Done. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:986: if (!cr.isMac) { On 2015/06/05 17:50:05, Rouslan wrote: > Make both handleSpellCheckDictionaryPrefChange_() and > handleSpellCheckDictionariesPrefChange_() do either one of "if (!cr.isMac) { do > work }" or "if (cr.isMac) return". The difference is jarring. Done. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:995: * @param {Event} e. Preference change event where e.value.value is the On 2015/06/05 17:50:05, Rouslan wrote: > No period (.) after the first mention of the variable name |e|. Done. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:1003: var commaSeparatedLanguageCodes = e.value.value; On 2015/06/05 17:50:05, Rouslan wrote: > Inline this variable into the next statement. Done. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:1007: for (var i = 0; i < languageCodesSplit.length; i++) On 2015/06/05 17:50:05, Rouslan wrote: > Easier to write this? > > for (var i in languageCodesSplit) > this.spellCheckLanguages_[languageCodesSplit[i]] = true; This isn't the right way to iterate over array elements. for-in loops, in Javascript, iterate over object properties and it goes down the prototype chain so it actually picks up array indices too. The style guide also says to use a normal for loop to iterate over arrays ( http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#for-in_... ). I'll leave it as it is. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:1058: if (this.spellCheckLanguages_.hasOwnProperty(currentLanguageCode)) On 2015/06/05 17:50:05, Rouslan wrote: > Are you able to iterate over the elements in this.spellCheckLanguages_ instead? Yep, this is the standard way of iterating over keys in javascript. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.cc (left): https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:132: languages->push_back(dictionary_language); On 2015/06/05 17:50:05, Rouslan wrote: > That's a lovely git diff. Acknowledged. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:99: user_prefs::UserPrefs::Get(const_cast<content::BrowserContext*>(context)); On 2015/06/05 17:50:05, Rouslan wrote: > Eh, const_cast like this is bad. Let's remove it and keep |context| non-const. > Sorry, I should've checked the user_prefs::UserPrefs::Get() API before my > previous recommendation. Done. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:115: size_t dictionary_size = languages->size(); On 2015/06/05 17:50:05, Rouslan wrote: > s/dictionary_size/enabled_spellcheck_languages > > Does this name make it more clear what's going on? Done. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.h (right): https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.h:66: // This function computes a vector of strings, |languages|, which are to be On 2015/06/05 17:50:05, Rouslan wrote: > // Computes |languages| to display in the context menu... Done. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.h:67: // displayed in the context menu over a text area for changing spell check On 2015/06/05 17:50:05, Rouslan wrote: > s/spell check/spellcheck > > Stick with your own conventions :-) Done. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.h:68: // languages. It returns the number of enabled spellcheck languages in On 2015/06/05 17:50:05, Rouslan wrote: > Not sure how to say this better, but let me try to shorten it: > > // Computes |languages| to display in the context menu over a text area for > changing spellcheck languages. Returns the number of the first spellcheck > |languages| that are enabled. > > (Alt-q in Sublime to reformat the comment paragraph. Lines should be at most 80 > char long.) Done. https://codereview.chromium.org/1156473007/diff/160001/chrome/common/spellche... File chrome/common/spellcheck_common.cc (right): https://codereview.chromium.org/1156473007/diff/160001/chrome/common/spellche... chrome/common/spellcheck_common.cc:202: auto empty_strings_begin = On 2015/06/05 17:50:05, Rouslan wrote: > Inline this variable into the next statement. Don't worry about formatting. > Clang-format is your friend. :-) Done. https://codereview.chromium.org/1156473007/diff/160001/chrome/common/spellche... chrome/common/spellcheck_common.cc:214: const base::CommandLine* command_line = On 2015/06/05 17:50:05, Rouslan wrote: > Inline this variable into the next statement. Again, use clang-format afterward. Done. https://codereview.chromium.org/1156473007/diff/160001/chrome/common/spellche... File chrome/common/spellcheck_common.h (right): https://codereview.chromium.org/1156473007/diff/160001/chrome/common/spellche... chrome/common/spellcheck_common.h:63: // Reads the spellcheck.dictionary or spellcheck.dictionaries preference On 2015/06/05 17:50:06, Rouslan wrote: > No need to specify so much implementation details, especially since > "spellcheck.dictionary" preference will eventually go away. > > // Returns the list of enabled spellcheck dictionaries. Done. https://codereview.chromium.org/1156473007/diff/160001/chrome/common/spellche... chrome/common/spellcheck_common.h:68: // Checks if the command-line switch enable-multilingual-spellchecker is set. On 2015/06/05 17:50:06, Rouslan wrote: > This comment will soon grow stale, because command-line will be only one of > several ways to enable multilingual spellchecker. > > // Returns true if multilingual spellchecker is enabled. Done. https://codereview.chromium.org/1156473007/diff/160001/chrome/common/spellche... chrome/common/spellcheck_common.h:69: bool MultilingualSpellcheckIsEnabled(); On 2015/06/05 17:50:06, Rouslan wrote: > Newline after. Done.
Great progress! Keep it up! https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resourc... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:26: * @const {string} On 2015/06/05 21:38:32, juliusa wrote: > On 2015/06/05 17:50:05, Rouslan wrote: > > Unless some Chrome tool or bot told you to do it this way, it's best to not > > modify notations here. There's a server that periodically runs Closure > compiler > > (I think) on these files and will bug you if you're loosey goosey with your > > comment conventions. > > I changed this because a comment on Klemen's patch mentioned to change it so I > just changed it wherever it showed up. I guess this is the newer way of doing it > (https://developers.google.com/closure/compiler/docs/js-for-compiler )? I could > change other similar comment styles in the file to match it or just change these > all back. Sounds good. let's leave it as is. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resourc... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:264: 'click', Will this catch taps on a touch screen? Many chromebooks have touchscreens these days. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_factory.cc (right): https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_factory.cc:7: #include "base/command_line.h" Unused include. There's an OK-ish tool to detect this kind of issues automatically: 'apt-get install iwyu'. (IWYU means include-what-you-use). I've integrated it into my emacs, but not sure about your Sublime. You can always try this from command line: $ (cd out/Release && `ninja -C. ../../chrome/browser/spellchecker/spellcheck_factory.cc^ -t commands | grep spellcheck_factory.cc | sed 's/.*clang++/include-what-you-use/'`) Take it with a grain of salt, however. This tools has some false positives and negatives from time to time. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_factory.cc:11: #include "chrome/common/chrome_switches.h" Unused include. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.h (right): https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.h:69: // |languages| will begin with all enabled spellcheck languages, followed by Delete lines 69 to 72. They seem redundant. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service_browsertest.cc (right): https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:50: class MultilingualSpellcheckServiceBrowserTest : public InProcessBrowserTest { Put MultilingualSpellcheckServiceBrowserTest right before IN_PROC_BROWSER_TEST_F(MultilingualSpellcheckServiceBrowserTest, GetSpellCheckLanguage). https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:111: prefs->SetString(prefs::kSpellCheckDictionaries, "fr,en-US"); Also set kSpellCheckDictionary to something invalid to make sure that value is not used. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:112: prefs->SetString(prefs::kAcceptLanguages, "fr,en-US,en-AU"); Mix up the order of the languages to differ from |languages| below. Include a language that does not have spellcheck dictionary. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:115: int ret = SpellcheckService::GetSpellCheckLanguages(context, &languages); size_t https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:117: ASSERT_EQ(ret, 2); EXPECT_EQ(2UL, ret); EXPECT because this will let the test continue and try other expectations. 2 has UL because size_t is a long and unsigned value. 2UL is on the left side because error messages from tests always say "Expected <left-side> but got <right-side>." https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:120: ASSERT_EQ("en-AU", languages[2]); ASSERT_EQ(3UL, languages.size()); EXPECT_EQ("fr", languages[0]); EXPECT_EQ("en-US", languages[1]); EXPECT_EQ("en-AU", languages[2]); https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:123: IN_PROC_BROWSER_TEST_F(SpellcheckServiceBrowserTest, GetSpellCheckLanguages) { Put IN_PROC_BROWSER_TEST_F(SpellcheckServiceBrowserTest, GetSpellCheckLanguages) together with other SpellcheckServiceBrowserTest tests. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:126: prefs->SetString(prefs::kSpellCheckDictionary, "fr"); Also set kSpellCheckDictionaries to something invalid to make sure that value is not used. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:130: int ret = SpellcheckService::GetSpellCheckLanguages(context, &languages); size_t https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:132: ASSERT_EQ(ret, 1); EXPECT_EQ(1UL, ret); https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:133: ASSERT_EQ("fr", languages[0]); ASSERT_EQ(1UL, languages.size()); EXPECT_EQ("fr", languages[0]); Assert is usually used to stop a test of a condition is violated. You usually stop a test because a failing condition will cause a crash, which you typically don't want in a test. (If |languages| is empty, then languages[0] will crash.) See https://code.google.com/p/googletest/wiki/Primer#Assertions https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service_unittest.cc (left): https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_unittest.cc:32: TEST_F(SpellcheckServiceTest, GetSpellCheckLanguages1) { Please port these tests to your new browser tests. (One of each tests for single language spellcheck and multilingual spellcheck.) https://codereview.chromium.org/1156473007/diff/180001/chrome/common/spellche... File chrome/common/spellcheck_common.cc (right): https://codereview.chromium.org/1156473007/diff/180001/chrome/common/spellche... chrome/common/spellcheck_common.cc:205: DCHECK(!language.empty()); Be sure to remove this DCHECK() when you submit this patch into the commit queue. DCHECK(!language.empty()); // TODO(juliusa): Remove before commit. We either DCHECK(condition) or handle the condition manually, but not both. In this case, the DHCECK() is useful during development to make sure that you're not accidentally adding empty strings to the list. After development is complete, handling empty strings is necessary for disk corruption. (I did not see any validity guarantees in prefs API. See https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC...
https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resourc... File chrome/browser/resources/options/language_options.html (right): https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resourc... chrome/browser/resources/options/language_options.html:45: id="language-options-spellcheck-language-checkbox-div"> Don't put tag names in IDs https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resourc... chrome/browser/resources/options/language_options.html:48: id="language-options-spellcheck-language-checkbox"> Is there another spellcheck language this would be confused with if we dropped the "language-options"? https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resourc... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:145: spellCheckDictionary_: '', Can we rename this to something other than dictionary? https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:264: 'click', On 2015/06/06 01:42:12, Rouslan wrote: > Will this catch taps on a touch screen? Many chromebooks have touchscreens these > days. Yes, but change is an arguably better event to listen for. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:668: } else if (languageCode == this.spellCheckDictionary_) { I'm confused. Is this comparing a string to a dictionary? https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:687: [spellCheckLanguageSection, dictionaryDownloadFailed], 1); Needs 2 more spaces https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:999: if (!cr.isMac) { Should this ever happen on Mac? assert(!cr.isMac); If it shouldn't and if (cr.isMac) return; If it does. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:1053: var languageCodes = []; I think this can be var languageCodes = Object.keys(this.spellCheckDictionaries_);
Patchset #8 (id:260001) has been deleted
Patchset #8 (id:280001) has been deleted
Patchset #6 (id:220001) has been deleted
Patchset #6 (id:240001) has been deleted
Patchset #6 (id:290001) has been deleted
Patchset #5 (id:200001) has been deleted
Patchset #5 (id:310001) has been deleted
Patchset #5 (id:330001) has been deleted
I rebuilt the patch focusing on getting the multilanguage browsertest to work and everything seems good. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resourc... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:264: 'click', On 2015/06/06 01:42:12, Rouslan wrote: > Will this catch taps on a touch screen? Many chromebooks have touchscreens these > days. Acknowledged. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_factory.cc (right): https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_factory.cc:7: #include "base/command_line.h" On 2015/06/06 01:42:12, Rouslan wrote: > Unused include. > > There's an OK-ish tool to detect this kind of issues automatically: 'apt-get > install iwyu'. (IWYU means include-what-you-use). I've integrated it into my > emacs, but not sure about your Sublime. You can always try this from command > line: > > $ (cd out/Release && `ninja -C. > ../../chrome/browser/spellchecker/spellcheck_factory.cc^ -t commands | grep > spellcheck_factory.cc | sed 's/.*clang++/include-what-you-use/'`) > > Take it with a grain of salt, however. This tools has some false positives and > negatives from time to time. Done. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_factory.cc:11: #include "chrome/common/chrome_switches.h" On 2015/06/06 01:42:12, Rouslan wrote: > Unused include. Done. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.h (right): https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.h:69: // |languages| will begin with all enabled spellcheck languages, followed by On 2015/06/06 01:42:12, Rouslan wrote: > Delete lines 69 to 72. They seem redundant. Done. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service_browsertest.cc (right): https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:50: class MultilingualSpellcheckServiceBrowserTest : public InProcessBrowserTest { On 2015/06/06 01:42:12, Rouslan wrote: > Put MultilingualSpellcheckServiceBrowserTest right before > IN_PROC_BROWSER_TEST_F(MultilingualSpellcheckServiceBrowserTest, > GetSpellCheckLanguage). Done. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:111: prefs->SetString(prefs::kSpellCheckDictionaries, "fr,en-US"); On 2015/06/06 01:42:13, Rouslan wrote: > Also set kSpellCheckDictionary to something invalid to make sure that value is > not used. It shouldn't be invalid. I'll add a JS test to click stuff and ensure it's OK. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:112: prefs->SetString(prefs::kAcceptLanguages, "fr,en-US,en-AU"); On 2015/06/06 01:42:12, Rouslan wrote: > Mix up the order of the languages to differ from |languages| below. > > Include a language that does not have spellcheck dictionary. Done. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:115: int ret = SpellcheckService::GetSpellCheckLanguages(context, &languages); On 2015/06/06 01:42:13, Rouslan wrote: > size_t Done. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:117: ASSERT_EQ(ret, 2); On 2015/06/06 01:42:13, Rouslan wrote: > EXPECT_EQ(2UL, ret); > > EXPECT because this will let the test continue and try other expectations. > > 2 has UL because size_t is a long and unsigned value. > > 2UL is on the left side because error messages from tests always say "Expected > <left-side> but got <right-side>." Done. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:120: ASSERT_EQ("en-AU", languages[2]); On 2015/06/06 01:42:13, Rouslan wrote: > ASSERT_EQ(3UL, languages.size()); > EXPECT_EQ("fr", languages[0]); > EXPECT_EQ("en-US", languages[1]); > EXPECT_EQ("en-AU", languages[2]); Done. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:123: IN_PROC_BROWSER_TEST_F(SpellcheckServiceBrowserTest, GetSpellCheckLanguages) { On 2015/06/06 01:42:13, Rouslan wrote: > Put IN_PROC_BROWSER_TEST_F(SpellcheckServiceBrowserTest, GetSpellCheckLanguages) > together with other SpellcheckServiceBrowserTest tests. Done. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:126: prefs->SetString(prefs::kSpellCheckDictionary, "fr"); On 2015/06/06 01:42:12, Rouslan wrote: > Also set kSpellCheckDictionaries to something invalid to make sure that value is > not used. It shouldn't be invalid. I'll add a JS test to click stuff and ensure it's OK. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:130: int ret = SpellcheckService::GetSpellCheckLanguages(context, &languages); On 2015/06/06 01:42:12, Rouslan wrote: > size_t Done. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:132: ASSERT_EQ(ret, 1); On 2015/06/06 01:42:12, Rouslan wrote: > EXPECT_EQ(1UL, ret); Done. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:133: ASSERT_EQ("fr", languages[0]); On 2015/06/06 01:42:12, Rouslan wrote: > ASSERT_EQ(1UL, languages.size()); > EXPECT_EQ("fr", languages[0]); > > Assert is usually used to stop a test of a condition is violated. You usually > stop a test because a failing condition will cause a crash, which you typically > don't want in a test. (If |languages| is empty, then languages[0] will crash.) > > See https://code.google.com/p/googletest/wiki/Primer#Assertions Done. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service_unittest.cc (left): https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_unittest.cc:32: TEST_F(SpellcheckServiceTest, GetSpellCheckLanguages1) { On 2015/06/06 01:42:13, Rouslan wrote: > Please port these tests to your new browser tests. (One of each tests for single > language spellcheck and multilingual spellcheck.) Done. https://codereview.chromium.org/1156473007/diff/180001/chrome/common/spellche... File chrome/common/spellcheck_common.cc (right): https://codereview.chromium.org/1156473007/diff/180001/chrome/common/spellche... chrome/common/spellcheck_common.cc:205: DCHECK(!language.empty()); On 2015/06/06 01:42:13, Rouslan wrote: > Be sure to remove this DCHECK() when you submit this patch into the commit > queue. > > DCHECK(!language.empty()); // TODO(juliusa): Remove before commit. > > We either DCHECK(condition) or handle the condition manually, but not both. In > this case, the DHCECK() is useful during development to make sure that you're > not accidentally adding empty strings to the list. After development is > complete, handling empty strings is necessary for disk corruption. (I did not > see any validity guarantees in prefs API. > > > See > https://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREAC... Done.
Updated some Javascript-related things. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resourc... File chrome/browser/resources/options/language_options.html (right): https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resourc... chrome/browser/resources/options/language_options.html:45: id="language-options-spellcheck-language-checkbox-div"> On 2015/06/09 17:57:47, Dan Beam wrote: > Don't put tag names in IDs Changed to "spellcheck-language-checkbox-container". https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resourc... chrome/browser/resources/options/language_options.html:48: id="language-options-spellcheck-language-checkbox"> On 2015/06/09 17:57:47, Dan Beam wrote: > Is there another spellcheck language this would be confused with if we dropped > the "language-options"? Nope. Done. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resourc... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:264: 'click', On 2015/06/09 17:57:47, Dan Beam wrote: > On 2015/06/06 01:42:12, Rouslan wrote: > > Will this catch taps on a touch screen? Many chromebooks have touchscreens > these > > days. > > Yes, but change is an arguably better event to listen for. Done. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:687: [spellCheckLanguageSection, dictionaryDownloadFailed], 1); On 2015/06/09 17:57:47, Dan Beam wrote: > Needs 2 more spaces Done. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:1053: var languageCodes = []; On 2015/06/09 17:57:47, Dan Beam wrote: > I think this can be > > var languageCodes = Object.keys(this.spellCheckDictionaries_); Done.
Awesome testing! https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:60: base::string16 display_name( Inline. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:62: if (chrome::spellcheck_common::MultilingualSpellcheckIsEnabled()) { Put the for loop inside of the if-statement, so you don't have to call MultilingualSpellcheckIsEnabled n times. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:131: static_cast<size_t>(command_id - IDC_SPELLCHECK_LANGUAGES_FIRST); Let's use base::strict_cast from https://code.google.com/p/chromium/codesearch#chromium/src/base/numerics/safe... https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_factory.cc (left): https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_factory.cc:58: These whitespace changes or not necessary. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_factory.cc (right): https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_factory.cc:73: // spellchecking and single-language spellchecking. You can put this on your own todo list, but let's not add TODOs in code :-) https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_factory.cc:79: } else { Is that an empty else{} block? https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:119: languages->end()) { If this is done by clang-format, please make a note in the code review. Otherwise, no need to change whitespace. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:123: return enabled_spellcheck_languages; Usually there's a newline after a "}" (unless it's followed by another "}"). https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service_browsertest.cc (right): https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:12: #include "chrome/browser/spellchecker/spellcheck_service.h" Please put this include at the top, followed by a newline. This is a good idea for unit tests to indicate which file they are testing. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:99: IN_PROC_BROWSER_TEST_F(SpellcheckServiceBrowserTest, GetSpellCheckLanguages1) { I know this is a copy from the unit test, but the rule is "if you touch it, you own it." This means you have to fix this... :-) ---------------------------------------------------------- Don't write 4 tests that essentially do the same thing. Use either parameterized tests or a kTestData struct with a loop. Parameterized test example: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libadd... Parameterized test explanation: https://code.google.com/p/googletest/wiki/AdvancedGuide#How_to_Write_Value-Pa... kTestData example: https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:105: std::vector<std::string> languages; s/languages/spellcheck_languages/ https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:106: size_t ret = SpellcheckService::GetSpellCheckLanguages(context, &languages); s/ret/enabled_spellcheck_languages/ https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:110: EXPECT_EQ("en-US", languages[0]); These two last two lines can be written simpler: EXPECT_EQ(std::vector(1, "en-US"), languages); std::vector(1, "en-US") is a constructor for std::vector with 1 element, which is "en-US". EXPECT_EQ knows how to compare two vectors. It will make sure that the vectors are of the same size. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.cc (right): https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.cc:23: auto setting_name = prefs::kLanguagePreferredLanguages; It's just an std::string. No need to auto such a short type. Auto is best used to replace something like: base::ScopedPtrHashMap<std::string, scoped_ptr<AutofillProfile>>::const_iterator https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.h (right): https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.h:15: void SetUpOnMainThread() override; Can be private. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.h:18: void SetUpCommandLine(base::CommandLine* command_line) override; Can be private. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js (right): https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:1: // Copyright 2014 The Chromium Authors. All rights reserved. 2015 https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:12: function MultilanguageOptionsWebUIBrowserTest() {} Is this necessary? https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:15: * Base fixture for Downloads WebUI testing. This comment is wrong. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:19: function BaseMultilanguageOptionsWebUIBrowserTest() {} Will it work to call this MultilanguageOptionsWebUIBrowserTest and remove "typedefCppFixture" below? https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:25: * Browse to the downloads page & call our preLoad(). That's not the "downloads" page. Please update the comment. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:43: expectEquals(this.browsePreload, document.location.href); It's best to use literal expected values in tests, even at the expense of repeated code. http://go/nologicintests expectEquals('chrome://settings-frame/languages/', document.location.href); https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:46: // Test that only certain languages can be selected and used for spellchecking. Specify the value of prefs::kLanguagePreferredLanguages/prefs::kAcceptLanguages in the comment. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:51: if (!cr.isMac) { I agree with Dan that you should assert(!cr.isMac). https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:55: expectEquals('en-US', loadTimeData.getString('currentUiLanguageCode')); I don't think you can rely on the UI locale being en-US. This test will fail for anyone in our Tokyo office, for example. https://codereview.chromium.org/1156473007/diff/350001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/1156473007/diff/350001/chrome/chrome_tests.gy... chrome/chrome_tests.gypi:540: 'browser/ui/webui/options/multilanguage_options_webui_browsertest.cc', Sort alphabetically, please. https://codereview.chromium.org/1156473007/diff/350001/chrome/common/spellche... File chrome/common/spellcheck_common.h (right): https://codereview.chromium.org/1156473007/diff/350001/chrome/common/spellche... chrome/common/spellcheck_common.h:15: class PrefService; namespace-less forward declaration should precede forward declarations inside of a namespace. https://codereview.chromium.org/1156473007/diff/350001/chrome/common/spellche... chrome/common/spellcheck_common.h:67: bool MultilingualSpellcheckIsEnabled(); I think it's more common to put "Is" at the front of the function name.
Patchset #6 (id:370001) has been deleted
Patchset #6 (id:390001) has been deleted
Refactored some old tests and addressed comments. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resourc... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:145: spellCheckDictionary_: '', On 2015/06/09 17:57:47, Dan Beam wrote: > Can we rename this to something other than dictionary? Renamed spellCheckDictionary_ and spellCheckDictionaries_ to spellCheckLanguage_ and spellCheckLanguages_ respectively. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:668: } else if (languageCode == this.spellCheckDictionary_) { On 2015/06/09 17:57:47, Dan Beam wrote: > I'm confused. Is this comparing a string to a dictionary? Variables have been renamed to eliminate confusion. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:999: if (!cr.isMac) { On 2015/06/09 17:57:47, Dan Beam wrote: > Should this ever happen on Mac? > > assert(!cr.isMac); > > If it shouldn't and > > if (cr.isMac) > return; > > If it does. It shouldn't. I added the assert. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:60: base::string16 display_name( On 2015/06/16 00:31:16, Rouslan wrote: > Inline. Done. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:62: if (chrome::spellcheck_common::MultilingualSpellcheckIsEnabled()) { On 2015/06/16 00:31:16, Rouslan wrote: > Put the for loop inside of the if-statement, so you don't have to call > MultilingualSpellcheckIsEnabled n times. Done. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:131: static_cast<size_t>(command_id - IDC_SPELLCHECK_LANGUAGES_FIRST); On 2015/06/16 00:31:16, Rouslan wrote: > Let's use base::strict_cast from > https://code.google.com/p/chromium/codesearch#chromium/src/base/numerics/safe... Done. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_factory.cc (left): https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_factory.cc:58: On 2015/06/16 00:31:16, Rouslan wrote: > These whitespace changes or not necessary. Done. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_factory.cc (right): https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_factory.cc:73: // spellchecking and single-language spellchecking. On 2015/06/16 00:31:16, Rouslan wrote: > You can put this on your own todo list, but let's not add TODOs in code :-) Done. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_factory.cc:79: } else { On 2015/06/16 00:31:16, Rouslan wrote: > Is that an empty else{} block? Done. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:119: languages->end()) { On 2015/06/16 00:31:16, Rouslan wrote: > If this is done by clang-format, please make a note in the code review. > Otherwise, no need to change whitespace. It was a clang-format thing. I'll note that in the future. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:123: return enabled_spellcheck_languages; On 2015/06/16 00:31:16, Rouslan wrote: > Usually there's a newline after a "}" (unless it's followed by another "}"). Done. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service_browsertest.cc (right): https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:12: #include "chrome/browser/spellchecker/spellcheck_service.h" On 2015/06/16 00:31:16, Rouslan wrote: > Please put this include at the top, followed by a newline. This is a good idea > for unit tests to indicate which file they are testing. Done. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:99: IN_PROC_BROWSER_TEST_F(SpellcheckServiceBrowserTest, GetSpellCheckLanguages1) { On 2015/06/16 00:31:16, Rouslan wrote: > I know this is a copy from the unit test, but the rule is "if you touch it, you > own it." This means you have to fix this... :-) > ---------------------------------------------------------- > Don't write 4 tests that essentially do the same thing. Use either parameterized > tests or a kTestData struct with a loop. > > Parameterized test example: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libadd... > > Parameterized test explanation: > https://code.google.com/p/googletest/wiki/AdvancedGuide#How_to_Write_Value-Pa... > > kTestData example: > https://code.google.com/p/chromium/codesearch#chromium/src/components/autofil... Done. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:105: std::vector<std::string> languages; On 2015/06/16 00:31:16, Rouslan wrote: > s/languages/spellcheck_languages/ Done. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:106: size_t ret = SpellcheckService::GetSpellCheckLanguages(context, &languages); On 2015/06/16 00:31:16, Rouslan wrote: > s/ret/enabled_spellcheck_languages/ Done. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:110: EXPECT_EQ("en-US", languages[0]); On 2015/06/16 00:31:16, Rouslan wrote: > These two last two lines can be written simpler: > > EXPECT_EQ(std::vector(1, "en-US"), languages); > > std::vector(1, "en-US") is a constructor for std::vector with 1 element, which > is "en-US". EXPECT_EQ knows how to compare two vectors. It will make sure that > the vectors are of the same size. Done. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.cc (right): https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.cc:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2015/06/16 00:31:17, Rouslan wrote: > 2015 Done. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.cc:23: auto setting_name = prefs::kLanguagePreferredLanguages; On 2015/06/16 00:31:17, Rouslan wrote: > It's just an std::string. No need to auto such a short type. Auto is best used > to replace something like: > > base::ScopedPtrHashMap<std::string, scoped_ptr<AutofillProfile>>::const_iterator Done. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.h (right): https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2015/06/16 00:31:17, Rouslan wrote: > 2015 Done. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.h:15: void SetUpOnMainThread() override; On 2015/06/16 00:31:17, Rouslan wrote: > Can be private. Done. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.h:18: void SetUpCommandLine(base::CommandLine* command_line) override; On 2015/06/16 00:31:17, Rouslan wrote: > Can be private. Done. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js (right): https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/06/16 00:31:17, Rouslan wrote: > 2015 Done. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:12: function MultilanguageOptionsWebUIBrowserTest() {} On 2015/06/16 00:31:17, Rouslan wrote: > Is this necessary? I renamed BaseMultilanguageOptionsWebUIBrowserTest to MultilanguageOptionsWebUIBrowserTest so now yes, it's necessary for the typedefCppFixture. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:15: * Base fixture for Downloads WebUI testing. On 2015/06/16 00:31:17, Rouslan wrote: > This comment is wrong. Done. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:19: function BaseMultilanguageOptionsWebUIBrowserTest() {} On 2015/06/16 00:31:17, Rouslan wrote: > Will it work to call this MultilanguageOptionsWebUIBrowserTest and remove > "typedefCppFixture" below? That generates compiler errors. The typedefCppFixture seems to need that blank JS function. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:25: * Browse to the downloads page & call our preLoad(). On 2015/06/16 00:31:17, Rouslan wrote: > That's not the "downloads" page. Please update the comment. Done. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:43: expectEquals(this.browsePreload, document.location.href); On 2015/06/16 00:31:17, Rouslan wrote: > It's best to use literal expected values in tests, even at the expense of > repeated code. http://go/nologicintests > > expectEquals('chrome://settings-frame/languages/', document.location.href); > Done. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:46: // Test that only certain languages can be selected and used for spellchecking. On 2015/06/16 00:31:17, Rouslan wrote: > Specify the value of prefs::kLanguagePreferredLanguages/prefs::kAcceptLanguages > in the comment. Done. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:51: if (!cr.isMac) { On 2015/06/16 00:31:17, Rouslan wrote: > I agree with Dan that you should assert(!cr.isMac). Done. https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:55: expectEquals('en-US', loadTimeData.getString('currentUiLanguageCode')); On 2015/06/16 00:31:17, Rouslan wrote: > I don't think you can rely on the UI locale being en-US. This test will fail for > anyone in our Tokyo office, for example. Done. https://codereview.chromium.org/1156473007/diff/350001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/1156473007/diff/350001/chrome/chrome_tests.gy... chrome/chrome_tests.gypi:540: 'browser/ui/webui/options/multilanguage_options_webui_browsertest.cc', On 2015/06/16 00:31:17, Rouslan wrote: > Sort alphabetically, please. Done. https://codereview.chromium.org/1156473007/diff/350001/chrome/common/spellche... File chrome/common/spellcheck_common.h (right): https://codereview.chromium.org/1156473007/diff/350001/chrome/common/spellche... chrome/common/spellcheck_common.h:15: class PrefService; On 2015/06/16 00:31:17, Rouslan wrote: > namespace-less forward declaration should precede forward declarations inside of > a namespace. Done. https://codereview.chromium.org/1156473007/diff/350001/chrome/common/spellche... chrome/common/spellcheck_common.h:67: bool MultilingualSpellcheckIsEnabled(); On 2015/06/16 00:31:17, Rouslan wrote: > I think it's more common to put "Is" at the front of the function name. Done. https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service_browsertest.cc (right): https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:33: const uint8 kCorruptedBDICT[] = { clang-format reformatted this. https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:118: bdict_path, reinterpret_cast<const char*>(kCorruptedBDICT), clang-format reformatted this and the lines below.
Awesome! So close to the finish line... https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_factory.cc (right): https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_factory.cc:78: user_prefs::PrefRegistrySyncable::SYNCABLE_PREF); prefs::kSpellCheckDictionary was not syncable originally. Let's not make prefs::kSpellCheckDictionaries syncable. https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_factory.cc:84: user_prefs::PrefRegistrySyncable::SYNCABLE_PREF); This was not syncable before. Let's not make it syncable. https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service_browsertest.cc (right): https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:12: #include "base/prefs/pref_member.h" Why this include? https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:33: const uint8 kCorruptedBDICT[] = { On 2015/06/17 00:59:53, juliusa wrote: > clang-format reformatted this. Acknowledged. https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:48: const unsigned long& expected_enabled_spellcheck_languages, size_t, no const, no &. "const var_type& var_name" (called const-ref) is used to avoid copying large chunks of memory. It's OK to use "var_type var_name" for pointers, int, long, size_t, char, etc, because they are cheap (fast) to copy. https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:59: const unsigned long expected_enabled_spellcheck_languages; size_t https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:67: Profile* GetProfile() { return browser()->profile(); } class SpellcheckServiceBrowserTest : public InProcessBrowserTest, public testing::WithParamInterface<SpellcheckLanguageTestCase> { public: SpellcheckServiceBrowserTest() {} ~SpellcheckServiceBrowserTest() override {} BrowserContext* GetContext() { return static_cast<BrowserContext*>(browser()->profile()); } private: DISALLOW_COPY_AND_ASSIGN(SpellcheckServiceBrowserTest); }; https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:85: std::vector<std::string>{"en-US", "fr", This is "(Uniform) Initialization Syntax", which is banned in Chromium at the moment: https://chromium-cpp.appspot.com/. Only 9 instances of this syntax can be found in the code, but they are all in third-party libraries that are not cross platform: https://code.google.com/p/chromium/codesearch#search/&q=std::vector%3C.*%3E%7... Your code needs to be cross platform. I recommend that you extend the SpellcheckLanguageTestCase constructor to take three StringPiece objects: https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin... By the way, the string_piece.h header comment says that it's preferred to pass StringPiece by value: "base::StringPiece first" etc. This is because StringPiece does some C++ magic to avoid copying the actual string around. Pretty neat! Anyway, once you have the three StringPiece objects in your SpellcheckLanguageTestCase constructor, do something like this: if (!first.empty()) expected_spellcheck_languages.push_back(first); if (!second.empty()) expected_spellcheck_languages.push_back(second); if (!third.empty()) expected_spellcheck_languages.push_back(third); https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:118: bdict_path, reinterpret_cast<const char*>(kCorruptedBDICT), On 2015/06/17 00:59:53, juliusa wrote: > clang-format reformatted this and the lines below. Acknowledged. https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:161: void SetUpCommandLine(base::CommandLine* command_line) override { Please also call "InProcBrowserTest::SetUpCommandLine(command_line);" https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.cc (right): https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.cc:10: #include "base/prefs/pref_service.h" is this include necessary? https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.cc:12: #include "chrome/browser/ui/browser_window.h" Is this include necessary? https://codereview.chromium.org/1156473007/diff/410001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/1156473007/diff/410001/chrome/chrome_tests.gy... chrome/chrome_tests.gypi:2352: # This tests the LanguageOptions UI which is not used on Mac. "This tests the language options UI features that do not exist on Mac." Mac has a language options UI, but it does not let you select spellcheck dictionaries. Chrome on Mac uses Mac's system-wide spelling checker, which is configurable through system preferences app.
Addressed comments, cleaned tests, removed unused includes. https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_factory.cc (right): https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_factory.cc:78: user_prefs::PrefRegistrySyncable::SYNCABLE_PREF); On 2015/06/17 17:47:15, Rouslan wrote: > prefs::kSpellCheckDictionary was not syncable originally. Let's not make > prefs::kSpellCheckDictionaries syncable. Done. https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_factory.cc:84: user_prefs::PrefRegistrySyncable::SYNCABLE_PREF); On 2015/06/17 17:47:15, Rouslan wrote: > This was not syncable before. Let's not make it syncable. Done. https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service_browsertest.cc (right): https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:12: #include "base/prefs/pref_member.h" On 2015/06/17 17:47:16, Rouslan wrote: > Why this include? Deleted. https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:48: const unsigned long& expected_enabled_spellcheck_languages, On 2015/06/17 17:47:15, Rouslan wrote: > size_t, no const, no &. > > "const var_type& var_name" (called const-ref) is used to avoid copying large > chunks of memory. It's OK to use "var_type var_name" for pointers, int, long, > size_t, char, etc, because they are cheap (fast) to copy. Done. https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:59: const unsigned long expected_enabled_spellcheck_languages; On 2015/06/17 17:47:15, Rouslan wrote: > size_t Done. https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:67: Profile* GetProfile() { return browser()->profile(); } On 2015/06/17 17:47:16, Rouslan wrote: > class SpellcheckServiceBrowserTest > : public InProcessBrowserTest, > public testing::WithParamInterface<SpellcheckLanguageTestCase> { > public: > SpellcheckServiceBrowserTest() {} > ~SpellcheckServiceBrowserTest() override {} > > BrowserContext* GetContext() { return > static_cast<BrowserContext*>(browser()->profile()); } > > private: > DISALLOW_COPY_AND_ASSIGN(SpellcheckServiceBrowserTest); > }; Done. https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:85: std::vector<std::string>{"en-US", "fr", On 2015/06/17 17:47:15, Rouslan wrote: > This is "(Uniform) Initialization Syntax", which is banned in Chromium at the > moment: https://chromium-cpp.appspot.com/. > > Only 9 instances of this syntax can be found in the code, but they are all in > third-party libraries that are not cross platform: > > https://code.google.com/p/chromium/codesearch#search/&q=std::vector%3C.*%3E%7... > > Your code needs to be cross platform. I recommend that you extend the > SpellcheckLanguageTestCase constructor to take three StringPiece objects: > > https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/strin... > > By the way, the string_piece.h header comment says that it's preferred to pass > StringPiece by value: "base::StringPiece first" etc. This is because StringPiece > does some C++ magic to avoid copying the actual string around. Pretty neat! > > Anyway, once you have the three StringPiece objects in your > SpellcheckLanguageTestCase constructor, do something like this: > > if (!first.empty()) > expected_spellcheck_languages.push_back(first); > > if (!second.empty()) > expected_spellcheck_languages.push_back(second); > > if (!third.empty()) > expected_spellcheck_languages.push_back(third); Done. https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:161: void SetUpCommandLine(base::CommandLine* command_line) override { On 2015/06/17 17:47:15, Rouslan wrote: > Please also call "InProcBrowserTest::SetUpCommandLine(command_line);" Done. https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.cc (right): https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.cc:10: #include "base/prefs/pref_service.h" On 2015/06/17 17:47:16, Rouslan wrote: > is this include necessary? pref_service is, browser_prefs is not. Removed. https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.cc:12: #include "chrome/browser/ui/browser_window.h" On 2015/06/17 17:47:16, Rouslan wrote: > Is this include necessary? Yes. I believe it's used to get a profile ( browser()->profile() ) https://codereview.chromium.org/1156473007/diff/410001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/1156473007/diff/410001/chrome/chrome_tests.gy... chrome/chrome_tests.gypi:2352: # This tests the LanguageOptions UI which is not used on Mac. On 2015/06/17 17:47:16, Rouslan wrote: > "This tests the language options UI features that do not exist on Mac." > > Mac has a language options UI, but it does not let you select spellcheck > dictionaries. Chrome on Mac uses Mac's system-wide spelling checker, which is > configurable through system preferences app. Done.
https://codereview.chromium.org/1156473007/diff/430001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service_browsertest.cc (right): https://codereview.chromium.org/1156473007/diff/430001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:51: accept_languages(accept_languages), +6 spaces of indent here and the one line below. https://codereview.chromium.org/1156473007/diff/430001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:93: base::StringPiece("en-US"), No need for base::StringPiece() it, Just "en-US" should do the trick.
Cleaned up the indentation ugliness and the unnecessary base::StringPiece constructors. https://codereview.chromium.org/1156473007/diff/430001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service_browsertest.cc (right): https://codereview.chromium.org/1156473007/diff/430001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:51: accept_languages(accept_languages), On 2015/06/17 21:15:07, Rouslan wrote: > +6 spaces of indent here and the one line below. Done. https://codereview.chromium.org/1156473007/diff/430001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:93: base::StringPiece("en-US"), On 2015/06/17 21:15:07, Rouslan wrote: > No need for base::StringPiece() it, Just "en-US" should do the trick. clang_format as well. Done.
lgtm % one problem below. You can send this out to OWNERs now. https://codereview.chromium.org/1156473007/diff/450001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.h (right): https://codereview.chromium.org/1156473007/diff/450001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.h:10: // This is a helper class used by downloads_ui_browsertest.js. This comment is wrong.
juliusa@google.com changed reviewers: + dbeam@chromium.org, groby@chromium.org
Picked up where Klemen left off, added some tests, and cleaned older code/
juliusa@google.com changed reviewers: - dbeam@chromium.org, groby@chromium.org
juliusa@google.com changed reviewers: + dbeam@chromium.org, groby@chromium.org
Rachel, PTAL at Patch Set #9, files chrome/browser/spellchecker/spellcheck_factory.cc chrome/browser/spellchecker/spellcheck_service.h chrome/browser/spellchecker/spellcheck_service.cc chrome/browser/spellchecker/spellcheck_service_browsertest.ccchrome/browser/spellchecker/spellcheck_service_unittest.cc
Dan, PTAL at Patch Set #9, files chrome/browser/resources/options/language_options.html chrome/browser/resources/options/language_options.js chrome/browser/ui/webui/options/language_options_dictionary_download_browsertest.js chrome/browser/ui/webui/options/language_options_handler_common.cc chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.h chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.cc chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js
isherman@chromium.org changed reviewers: + isherman@chromium.org
histograms.xml lgtm
https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/resourc... File chrome/browser/resources/options/language_options.html (right): https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/resourc... chrome/browser/resources/options/language_options.html:45: id="spellcheck-language-checkbox-container"> unwrap, i.e. <div class="checkbox" id="spellcheck-language-checkbox-container"> https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/resourc... chrome/browser/resources/options/language_options.html:48: id="spellcheck-language-checkbox"> also unwrap this https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/resourc... chrome/browser/resources/options/language_options.html:122: <div id="spellcheck-option" class="checkbox"> unused ID https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/resourc... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:261: var spellCheckLanguageCheckbox = getRequiredElement( nit: $('spellcheck-language-checkbox').addEventListener( is functionally the same and shorter. getRequiredElement() is the same as assert($()). because you use the result directly after querying for that node, the code would blow up spectacularly anyways, so I don't think assert() helps much here. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:658: if (!languageCode) when does this happen? https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:665: this.spellCheckLanguages_.hasOwnProperty(languageCode); +\s\s https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:665: this.spellCheckLanguages_.hasOwnProperty(languageCode); note: this works even if this.spellCheckLanguages_['fr'] === false, but that supposedly can't happen at the moment https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:1000: var languageCodesSplit = e.value.value.split(','); nit: languages or languageCodes https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:1458: languageCode); +\s\s https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:1458: languageCode); nit: my eyes have always thought this is prettier var instance = LanguageOptions.getInstance(); instance.updateSpellCheckLanguageControls_(languageCode); https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.cc (right): https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.cc:11: #include "chrome/browser/ui/browser_window.h" why is this needed? https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.h (right): https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.h:7: nit: #include "base/macros.h" for DISALLOW_COPY_AND_ASSIGN() https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.h:20: void SetUpOnMainThread() override; nit: \n https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js (right): https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:26: runAccessibilityChecks: true, ^ unnecessary (true is already the default) https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:34: 'testOpenLanguageOptions', TEST_F('MultilanguageOptionsWebUIBrowserTest', 'testOpenLanguageOptions', function() { https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:44: function() { TEST_F('MultilanguageOptionsWebUIBrowserTest', 'languageOptions', function() { https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:45: expectTrue(loadTimeData.getBoolean('enableMultilingualSpellChecker')); assertTrue(loadTimeData.getBoolean('enableMultilingualSpellChecker')); https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:46: assert(!cr.isMac); assertFalse(cr.isMac); https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:49: var langOps = LanguageOptions.getInstance(); langOpts https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:51: // Make sure the only language currently selected is 'fr'. that's not what this code is doing var langs = Object.keys(langOps.spellCheckLanguages_); expectEquals(1, langs.length); expectEquals('fr', langs[0]); https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:99: expectFalse(langOps.spellCheckLanguages_.hasOwnProperty('en')); split these paragraphs into multiple tests with a comment setUp method
https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h (right): https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h:48: size_t selected_languages_; num_selected_languages, please. (Or something else indicating this is a count) https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:62: submenu_model_.AddCheckItem( Weird question: Where do we initialize which languages are enabled or not? https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:135: base::checked_cast<size_t>(command_id - Please don't - this adds a CHECK statement, which adds cost to Chrome's runtime - without any need to do so. The if-command above already guarantees that the value will be within an acceptable range. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_factory.cc (right): https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_factory.cc:75: user_prefs->RegisterStringPref( Why do we need a second stringpref? https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_factory.cc:83: Since there wasn't a newline before, let's not add one. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.h (right): https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.h:68: // |languages| that are enabled. What does "returns the number of the first... that are enabled" mean? https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service_browsertest.cc (right): https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:33: 0x42, 0x44, 0x69, 0x63, 0x02, 0x00, 0x01, 0x00, 0x20, 0x00, 0x00, 0x00, Please don't reformat this when none of the data has changed. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:47: base::StringPiece first, StringPiece is nice, and very performant, and overkill here. As somebody who reads the test, all I care about is that a string is passed here - there's no need for StringPiece. Also, just pass in a list of comma-separated values and split them in here - it's much easier to read that than a long parameter list. (Plus, it's already done implicitly for accept_languages, so you have a mix of styles in the API) https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:73: public testing::WithParamInterface<SpellcheckLanguageTestCase> { While WithParamInterface is clever - is the error output actually meaningful here if one of the tests fails? https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:89: testing::Values(SpellcheckLanguageTestCase("en-US", It seems to me these tests all test different facets of the language system. If a test fails, will somebody not familiar with the code be able to reasonably fast figure out why it failed? https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:99: nullptr, Don't use a nullptr here. Yes, StringPiece handles it like an empty string, but an empty string is a bit more readable. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:116: PrefService* prefs = user_prefs::UserPrefs::Get(context); This is pure setup for the test case - can this move into the fixture's SetUp()? https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:178: class MultilingualSpellcheckServiceBrowserTest Can't this just inherit from the fixture above? https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service_unittest.cc (left): https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_unittest.cc:34: TEST_F(SpellcheckServiceTest, GetSpellCheckLanguages1) { This is not good - in general, Chrome prefers unit tests over browser tests. Is there a way to avoid moving these into browsertests?
Patchset #10 (id:490001) has been deleted
Patchset #10 (id:500025) has been deleted
https://codereview.chromium.org/1156473007/diff/540001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js (right): https://codereview.chromium.org/1156473007/diff/540001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:29: setUp: function() { testing.Test.prototype.setUp.call(this);
Patchset #10 (id:520001) has been deleted
Julius, I noticed that unselecting all languages does not stop spellchecker from working. You can take care of this in part 3 of your project (making sure that spellchecker follows the multilingual settings.) Also note a correction below. Grayed out "ask google for suggestions" should not have a checkmark. https://codereview.chromium.org/1156473007/diff/580001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/spelling_menu_observer.cc (right): https://codereview.chromium.org/1156473007/diff/580001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spelling_menu_observer.cc:209: return integrate_spelling_service_.GetValue() && !profile->IsOffTheRecord(); && !chrome::spellcheck_common::IsMultilingualSpellcheckEnabled();
Patchset #13 (id:600001) has been deleted
Patchset #13 (id:620001) has been deleted
Patchset #13 (id:640001) has been deleted
Changes look good! I did not find any bugs when testing this. Just a few minor style nits below. Oh, and I think we need a multilingual_spellchecker_options_browsertest.cc instead of adding --enable-multilingual-spellchecker to all tests in options_browsertest.cc. (Unless you know how to launch a single test case with the command line flag.) https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:47: std::string locale; Rename to spellcheck_language. https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:48: std::string language_code; Move to right above GetISOLanguageCountryCodeFromLocale(). https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:49: std::string country_code; Move to right above GetISOLanguageCountryCodeFromLocale(). https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:84: #endif #endif // !defined(OS_MACOSX) https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:85: if (!chrome::spellcheck_common::IsMultilingualSpellcheckEnabled()) { Add one empty line after #endif for clarity. https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:90: } Add one empty line after } for clarity. https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:135: languages->end()) { Did clang-format do this? I keep forgetting. https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:142: #endif // !OS_MACOSX #endif // !defined(OS_MACOSX) https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:295: if (dictionary_languages <= 0) { size_t is unsigned, so it cannot be < 0. http://www.cplusplus.com/reference/cstring/size_t/ https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:299: } Chromium's C++ code usually does not have curlies {} for single-line if-statments: if (dictionary_languages > 0) dictionary = languages[0]; else return; https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:302: ->GetString(prefs::kSpellCheckDictionary); Is this clang-format's doing? https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:315: #endif #endif // !defined(OS_MACOSX) https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.h (right): https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.h:147: // Reacts to a change in user preference on which language should be used for Move the comment inside of #if https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/options_browsertest.cc (right): https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/options_browsertest.cc:74: void OptionsBrowserTest::SetUpCommandLine( Not a good idea to append --enable-multilingual-spellchecker to all tests for chrome://settings. Unless you know how to make one test case flip the flag, you probably need toadd a multilingual_spellchecker_options_browsertest.cc and the corresponding .js file. https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/options_browsertest.h (right): https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/options_browsertest.h:10: #include "base/macros.h" Did iwyu suggest this include? https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/options_browsertest.h:44: void SetUpCommandLine(base::CommandLine* command_line) override; All overrides are usually grouped together per parent. SetUpCommandLine is defined in BrowserTestBase, which is an ancestor of WebUIBrowserTest. So, this override should be on line 32. It does not need its own comment.
Patchset #14 (id:680001) has been deleted
Patchset #14 (id:700001) has been deleted
Patchset #11 (id:560001) has been deleted
A couple of notes. https://codereview.chromium.org/1156473007/diff/740001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1156473007/diff/740001/chrome/browser/about_f... chrome/browser/about_flags.cc:2005: "enable-multilingual-spellchecker", previous entry is clang-formatted. Maybe clang-format works in this file now? https://codereview.chromium.org/1156473007/diff/740001/chrome/browser/about_f... chrome/browser/about_flags.cc:2012: // NOTE: Adding new command-line switches requires adding corresponding I think the original comment had 4 spaces. Let's not change it. https://codereview.chromium.org/1156473007/diff/740001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/1156473007/diff/740001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:86: // Add a check item "Ask Google for spelling suggestions" item if multilingual No need to say "if multilingual spellchecking is not enabled." That's clear from " if (!chrome::spellcheck_common::IsMultilingualSpellcheckEnabled())" on hte next line. https://codereview.chromium.org/1156473007/diff/740001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_factory.cc (right): https://codereview.chromium.org/1156473007/diff/740001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_factory.cc:77: l10n_util::GetStringUTF8(IDS_SPELLCHECK_DICTIONARY)); This statement is hard to read. Would it be easier if you clang-formatted this instead? user_prefs->RegisterPref( chrome::spellcheck_common::IsMultilingualSpellcheckEnabled() ? prefs::kSpellCheckDictionaries : prefs::kSpellCheckDictionary, l10n_util::GetStringUTF8(IDS_SPELLCHECK_DICTIONARY)); https://codereview.chromium.org/1156473007/diff/740001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/1156473007/diff/740001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:52: spellcheck_language = dictionary_languages[0]; Please handle the case when dictionary_languages is empty. https://codereview.chromium.org/1156473007/diff/740001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:291: dictionary = languages[0]; Please handle the case of empty |languages| or DCHECK() that it's not empty (when dictionary_languages > 0).
https://codereview.chromium.org/1156473007/diff/760001/chrome/browser/resourc... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/760001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:999: if (languages[i] !== '') In which case would |languages[i]| be empty? Is this a bug elsewhere in your code or can be explained in some other way? https://codereview.chromium.org/1156473007/diff/760001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/1156473007/diff/760001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:55: spellcheck_language = ""; No need for else. spellcheck_language is already an empty string. https://codereview.chromium.org/1156473007/diff/760001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:297: dictionary = ""; No need for else. |dictionary| is already an empty string.
Rachel, PTAL at Patch Set #9, files chrome/browser/spellchecker/spellcheck_factory.cc chrome/browser/spellchecker/spellcheck_service.h chrome/browser/spellchecker/spellcheck_service.cc chrome/browser/spellchecker/spellcheck_service_browsertest.ccchrome/browser/spellchecker/spellcheck_service_unittest.cc https://codereview.chromium.org/1156473007/diff/450001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.h (right): https://codereview.chromium.org/1156473007/diff/450001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.h:10: // This is a helper class used by downloads_ui_browsertest.js. On 2015/06/17 22:08:36, Rouslan wrote: > This comment is wrong. Done. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h (right): https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h:48: size_t selected_languages_; On 2015/06/18 00:44:29, groby wrote: > num_selected_languages, please. (Or something else indicating this is a count) Done. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:62: submenu_model_.AddCheckItem( On 2015/06/18 00:44:29, groby wrote: > Weird question: Where do we initialize which languages are enabled or not? In SpellCheckService::GetSpellCheckLanguages we pull out the PrefMember for kAcceptLanguages. Then in spellcheck_common GetDictionaryLanguagesPref we pick out the languages from the kSpellCheckDictionary/kSpellCheckDictionaries pref. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:135: base::checked_cast<size_t>(command_id - On 2015/06/18 00:44:29, groby wrote: > Please don't - this adds a CHECK statement, which adds cost to Chrome's runtime > - without any need to do so. > > The if-command above already guarantees that the value will be within an > acceptable range. Reverted to static_cast<size_t> https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/resourc... File chrome/browser/resources/options/language_options.html (right): https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/resourc... chrome/browser/resources/options/language_options.html:45: id="spellcheck-language-checkbox-container"> On 2015/06/17 23:42:00, Dan Beam wrote: > unwrap, i.e. > > <div class="checkbox" id="spellcheck-language-checkbox-container"> Done. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/resourc... chrome/browser/resources/options/language_options.html:48: id="spellcheck-language-checkbox"> On 2015/06/17 23:42:00, Dan Beam wrote: > also unwrap this Done. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/resourc... chrome/browser/resources/options/language_options.html:122: <div id="spellcheck-option" class="checkbox"> On 2015/06/17 23:42:00, Dan Beam wrote: > unused ID Removed id attribute. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/resourc... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:261: var spellCheckLanguageCheckbox = getRequiredElement( On 2015/06/17 23:42:01, Dan Beam wrote: > nit: > > $('spellcheck-language-checkbox').addEventListener( > > is functionally the same and shorter. > > getRequiredElement() is the same as assert($()). because you use the result > directly after querying for that node, the code would blow up spectacularly > anyways, so I don't think assert() helps much here. Done. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:658: if (!languageCode) On 2015/06/17 23:42:01, Dan Beam wrote: > when does this happen? It shouldn't. Maybe it was precautionary code. Deleted. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:665: this.spellCheckLanguages_.hasOwnProperty(languageCode); On 2015/06/17 23:42:00, Dan Beam wrote: > +\s\s Done. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:665: this.spellCheckLanguages_.hasOwnProperty(languageCode); On 2015/06/17 23:42:00, Dan Beam wrote: > note: this works even if this.spellCheckLanguages_['fr'] === false, but that > supposedly can't happen at the moment The value of a key in spellCheckLanguages_ should never be false. We just want to make sure the key exists in the spellCheckLanguages_ object. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:1000: var languageCodesSplit = e.value.value.split(','); On 2015/06/17 23:42:01, Dan Beam wrote: > nit: languages or languageCodes languages it is. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:1458: languageCode); On 2015/06/17 23:42:01, Dan Beam wrote: > nit: my eyes have always thought this is prettier > > var instance = LanguageOptions.getInstance(); > instance.updateSpellCheckLanguageControls_(languageCode); Done. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:1458: languageCode); On 2015/06/17 23:42:01, Dan Beam wrote: > +\s\s With change above, I think this is unnecessary. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_factory.cc (right): https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_factory.cc:75: user_prefs->RegisterStringPref( On 2015/06/18 00:44:29, groby wrote: > Why do we need a second stringpref? This has been fixed. We now only set one or the other depending on if multilingual spellchecking is enabled or not. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_factory.cc:83: On 2015/06/18 00:44:29, groby wrote: > Since there wasn't a newline before, let's not add one. Done. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.h (right): https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.h:68: // |languages| that are enabled. On 2015/06/18 00:44:29, groby wrote: > What does "returns the number of the first... that are enabled" mean? The strings at the beinning of |languages| are currently enabled for spellchecking. This function returns how many of those there are. I've tried to clarify the comment. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service_browsertest.cc (right): https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:33: 0x42, 0x44, 0x69, 0x63, 0x02, 0x00, 0x01, 0x00, 0x20, 0x00, 0x00, 0x00, On 2015/06/18 00:44:29, groby wrote: > Please don't reformat this when none of the data has changed. clang-format changed it. I've reverted it. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:47: base::StringPiece first, On 2015/06/18 00:44:29, groby wrote: > StringPiece is nice, and very performant, and overkill here. > > As somebody who reads the test, all I care about is that a string is passed here > - there's no need for StringPiece. > > Also, just pass in a list of comma-separated values and split them in here - > it's much easier to read that than a long parameter list. (Plus, it's already > done implicitly for accept_languages, so you have a mix of styles in the API) I've implemented the csv approach instead. Done. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:73: public testing::WithParamInterface<SpellcheckLanguageTestCase> { On 2015/06/18 00:44:29, groby wrote: > While WithParamInterface is clever - is the error output actually meaningful > here if one of the tests fails? Yes, it tells you which test and the number of the case it failed on (of the form GetSpellcheckLanguages/#) and it tells you which condition failed and what the values were that caused it to fail. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:89: testing::Values(SpellcheckLanguageTestCase("en-US", On 2015/06/18 00:44:30, groby wrote: > It seems to me these tests all test different facets of the language system. If > a test fails, will somebody not familiar with the code be able to reasonably > fast figure out why it failed? I think these different cases of the GetSpellCheckLanguages function and the fact that the parameterized tests show expected and actual output will make it easy for people to see the error. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:99: nullptr, On 2015/06/18 00:44:29, groby wrote: > Don't use a nullptr here. Yes, StringPiece handles it like an empty string, but > an empty string is a bit more readable. Not using StringPiece anymore. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:116: PrefService* prefs = user_prefs::UserPrefs::Get(context); On 2015/06/18 00:44:29, groby wrote: > This is pure setup for the test case - can this move into the fixture's SetUp()? Not doing it this way anymore now that it's a unit test but I've moved some things into a SetUp method. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_browsertest.cc:178: class MultilingualSpellcheckServiceBrowserTest On 2015/06/18 00:44:30, groby wrote: > Can't this just inherit from the fixture above? Done. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service_unittest.cc (left): https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_unittest.cc:34: TEST_F(SpellcheckServiceTest, GetSpellCheckLanguages1) { On 2015/06/18 00:44:30, groby wrote: > This is not good - in general, Chrome prefers unit tests over browser tests. Is > there a way to avoid moving these into browsertests? Done. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.cc (right): https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.cc:11: #include "chrome/browser/ui/browser_window.h" On 2015/06/17 23:42:01, Dan Beam wrote: > why is this needed? It's included for browser()->profile() https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.h (right): https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.h:7: On 2015/06/17 23:42:01, Dan Beam wrote: > nit: > > #include "base/macros.h" > > for DISALLOW_COPY_AND_ASSIGN() Done. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.h:20: void SetUpOnMainThread() override; On 2015/06/17 23:42:01, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js (right): https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:26: runAccessibilityChecks: true, On 2015/06/17 23:42:01, Dan Beam wrote: > ^ unnecessary (true is already the default) Deleted. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:34: 'testOpenLanguageOptions', On 2015/06/17 23:42:01, Dan Beam wrote: > TEST_F('MultilanguageOptionsWebUIBrowserTest', 'testOpenLanguageOptions', > function() { Done. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:44: function() { On 2015/06/17 23:42:01, Dan Beam wrote: > TEST_F('MultilanguageOptionsWebUIBrowserTest', 'languageOptions', > function() { Done. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:45: expectTrue(loadTimeData.getBoolean('enableMultilingualSpellChecker')); On 2015/06/17 23:42:01, Dan Beam wrote: > assertTrue(loadTimeData.getBoolean('enableMultilingualSpellChecker')); Done. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:46: assert(!cr.isMac); On 2015/06/17 23:42:01, Dan Beam wrote: > assertFalse(cr.isMac); Done. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:49: var langOps = LanguageOptions.getInstance(); On 2015/06/17 23:42:01, Dan Beam wrote: > langOpts Done. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:51: // Make sure the only language currently selected is 'fr'. On 2015/06/17 23:42:01, Dan Beam wrote: > that's not what this code is doing > > var langs = Object.keys(langOps.spellCheckLanguages_); > expectEquals(1, langs.length); > expectEquals('fr', langs[0]); Done. https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:99: expectFalse(langOps.spellCheckLanguages_.hasOwnProperty('en')); On 2015/06/17 23:42:01, Dan Beam wrote: > split these paragraphs into multiple tests with a comment setUp method Done. https://codereview.chromium.org/1156473007/diff/540001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js (right): https://codereview.chromium.org/1156473007/diff/540001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:29: setUp: function() { On 2015/06/19 20:43:42, Dan Beam wrote: > testing.Test.prototype.setUp.call(this); Done. https://codereview.chromium.org/1156473007/diff/580001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/spelling_menu_observer.cc (right): https://codereview.chromium.org/1156473007/diff/580001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spelling_menu_observer.cc:209: return integrate_spelling_service_.GetValue() && !profile->IsOffTheRecord(); On 2015/06/21 23:43:44, Rouslan wrote: > && !chrome::spellcheck_common::IsMultilingualSpellcheckEnabled(); Done. https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:47: std::string locale; On 2015/06/23 00:43:05, Rouslan wrote: > Rename to spellcheck_language. Done. https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:48: std::string language_code; On 2015/06/23 00:43:06, Rouslan wrote: > Move to right above GetISOLanguageCountryCodeFromLocale(). Done. https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:49: std::string country_code; On 2015/06/23 00:43:05, Rouslan wrote: > Move to right above GetISOLanguageCountryCodeFromLocale(). Done. https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:84: #endif On 2015/06/23 00:43:06, Rouslan wrote: > #endif // !defined(OS_MACOSX) Done. https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:85: if (!chrome::spellcheck_common::IsMultilingualSpellcheckEnabled()) { On 2015/06/23 00:43:06, Rouslan wrote: > Add one empty line after #endif for clarity. Done. https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:90: } On 2015/06/23 00:43:06, Rouslan wrote: > Add one empty line after } for clarity. Done. https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:135: languages->end()) { On 2015/06/23 00:43:06, Rouslan wrote: > Did clang-format do this? I keep forgetting. Yeah, clang-format did this. https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:142: #endif // !OS_MACOSX On 2015/06/23 00:43:06, Rouslan wrote: > #endif // !defined(OS_MACOSX) Done. https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:295: if (dictionary_languages <= 0) { On 2015/06/23 00:43:06, Rouslan wrote: > size_t is unsigned, so it cannot be < 0. > > http://www.cplusplus.com/reference/cstring/size_t/ Done. https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:299: } On 2015/06/23 00:43:05, Rouslan wrote: > Chromium's C++ code usually does not have curlies {} for single-line > if-statments: > > if (dictionary_languages > 0) > dictionary = languages[0]; > else > return; Done. https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:302: ->GetString(prefs::kSpellCheckDictionary); On 2015/06/23 00:43:06, Rouslan wrote: > Is this clang-format's doing? Yep, clang-format. https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:315: #endif On 2015/06/23 00:43:06, Rouslan wrote: > #endif // !defined(OS_MACOSX) Done. https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.h (right): https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.h:147: // Reacts to a change in user preference on which language should be used for On 2015/06/23 00:43:06, Rouslan wrote: > Move the comment inside of #if Done. https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/options_browsertest.cc (right): https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/options_browsertest.cc:74: void OptionsBrowserTest::SetUpCommandLine( On 2015/06/23 00:43:06, Rouslan wrote: > Not a good idea to append --enable-multilingual-spellchecker to all tests for > chrome://settings. Unless you know how to make one test case flip the flag, you > probably need toadd a multilingual_spellchecker_options_browsertest.cc and the > corresponding .js file. I just created a MultilanguageOptionsBrowserTest test class in options_browsertest.js that reuses the typedefCppFixture from multilanguage_options_webui_browsertest.js. https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/options_browsertest.h (right): https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/options_browsertest.h:10: #include "base/macros.h" On 2015/06/23 00:43:06, Rouslan wrote: > Did iwyu suggest this include? In a previous comment, Dan said to include it for the DISALLOW_COPY_AND_ASSIGN macro. https://codereview.chromium.org/1156473007/diff/660001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/options_browsertest.h:44: void SetUpCommandLine(base::CommandLine* command_line) override; On 2015/06/23 00:43:06, Rouslan wrote: > All overrides are usually grouped together per parent. SetUpCommandLine is > defined in BrowserTestBase, which is an ancestor of WebUIBrowserTest. So, this > override should be on line 32. It does not need its own comment. Done. https://codereview.chromium.org/1156473007/diff/740001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1156473007/diff/740001/chrome/browser/about_f... chrome/browser/about_flags.cc:2005: "enable-multilingual-spellchecker", On 2015/06/23 21:56:01, Rouslan wrote: > previous entry is clang-formatted. Maybe clang-format works in this file now? It reformats the whole file but I've reformatted my part to look like everything around it. https://codereview.chromium.org/1156473007/diff/740001/chrome/browser/about_f... chrome/browser/about_flags.cc:2012: // NOTE: Adding new command-line switches requires adding corresponding On 2015/06/23 21:56:00, Rouslan wrote: > I think the original comment had 4 spaces. Let's not change it. Done. https://codereview.chromium.org/1156473007/diff/740001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/1156473007/diff/740001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:86: // Add a check item "Ask Google for spelling suggestions" item if multilingual On 2015/06/23 21:56:01, Rouslan wrote: > No need to say "if multilingual spellchecking is not enabled." That's clear from > " if (!chrome::spellcheck_common::IsMultilingualSpellcheckEnabled())" on hte > next line. Done. https://codereview.chromium.org/1156473007/diff/740001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_factory.cc (right): https://codereview.chromium.org/1156473007/diff/740001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_factory.cc:77: l10n_util::GetStringUTF8(IDS_SPELLCHECK_DICTIONARY)); On 2015/06/23 21:56:01, Rouslan wrote: > This statement is hard to read. Would it be easier if you clang-formatted this > instead? > > user_prefs->RegisterPref( > chrome::spellcheck_common::IsMultilingualSpellcheckEnabled() > ? prefs::kSpellCheckDictionaries > : prefs::kSpellCheckDictionary, > l10n_util::GetStringUTF8(IDS_SPELLCHECK_DICTIONARY)); Done. https://codereview.chromium.org/1156473007/diff/740001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/1156473007/diff/740001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:52: spellcheck_language = dictionary_languages[0]; On 2015/06/23 21:56:01, Rouslan wrote: > Please handle the case when dictionary_languages is empty. Done. https://codereview.chromium.org/1156473007/diff/740001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:291: dictionary = languages[0]; On 2015/06/23 21:56:01, Rouslan wrote: > Please handle the case of empty |languages| or DCHECK() that it's not empty > (when dictionary_languages > 0). Done. https://codereview.chromium.org/1156473007/diff/760001/chrome/browser/resourc... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/760001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:999: if (languages[i] !== '') On 2015/06/24 20:35:10, Rouslan wrote: > In which case would |languages[i]| be empty? Is this a bug elsewhere in your > code or can be explained in some other way? e.value.value can be an empty string and splitting an empty string will give ['']. We don't want an empty language in the spellCheckLanguages_ object. https://codereview.chromium.org/1156473007/diff/760001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/1156473007/diff/760001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:55: spellcheck_language = ""; On 2015/06/24 20:35:10, Rouslan wrote: > No need for else. spellcheck_language is already an empty string. Done. https://codereview.chromium.org/1156473007/diff/760001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:297: dictionary = ""; On 2015/06/24 20:35:10, Rouslan wrote: > No need for else. |dictionary| is already an empty string. Done.
Rachel, PTAL at Patch Set #16, please excuse the last email incorrectly stating Patch Set #9, files chrome/browser/spellchecker/spellcheck_factory.cc chrome/browser/spellchecker/spellcheck_service.h chrome/browser/spellchecker/spellcheck_service.cc chrome/browser/spellchecker/spellcheck_service_browsertest.ccchrome/browser/spellchecker/spellcheck_service_unittest.cc
Dan, PTAL at Patch Set #16, files chrome/browser/resources/options/browser_options.html chrome/browser/resources/options/language_options.html chrome/browser/resources/options/language_options.js chrome/browser/resources/options/options.js chrome/browser/ui/webui/options/language_options_dictionary_download_browsertest.js chrome/browser/ui/webui/options/language_options_handler_common.cc chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.h chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.cc chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js
Mostly still high-level questions https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_factory.cc (right): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_factory.cc:75: chrome::spellcheck_common::IsMultilingualSpellcheckEnabled() Hrmpfh. That means we're leaving an old pref lying around. I'd rather we migrate dictionaries to a list, independent of IsMultilingual. (I'm not sure we _can_ migrate types for preferences, though. Which would be sad...) I'd also, orthogonal to _that_ decision, like the multi-dictionary method to be a ListPref https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.cc (left): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:108: if (spellcheck_mac::SpellCheckerAvailable()) Why are we getting rid of the OSX path? https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:49: std::vector<std::string> dictionary_languages; As said above, please use a list pref. Preferences have built-in support for lists, we shouldn't hand-code our own formats on top of preferences if we don't have to. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:115: base::SupportsUserData* context, Why move this to SupportsUserData? https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:288: if (chrome::spellcheck_common::IsMultilingualSpellcheckEnabled()) { If we migrated prefs, that would turn into a single code path... https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service_unittest.cc (right): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_unittest.cc:23: struct SpellcheckLanguageTestCase { FWIW, you're both adding new test cases and turning the old test cases into a TEST_CASE_P - you might want to factor out the latter and do that in a separate CL. It'll make reviewing and understanding changes easier. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/spellch... File chrome/browser/spellchecker/spelling_service_client.cc (right): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/spellch... chrome/browser/spellchecker/spelling_service_client.cc:119: // multilingual spellchecking is enabled the spelling service should be Why do we not want the spelling service for multilingual checking?
https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... File chrome/browser/resources/options/browser_options.html (right): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... chrome/browser/resources/options/browser_options.html:419: class="checkbox controlled-setting-with-label"> 4 \s indent https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... chrome/browser/resources/options/browser_options.html:434: <if expr="_google_chrome"> this should probably be above <div id="metrics-reporting-setting"> https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... chrome/browser/resources/options/browser_options.html:434: <if expr="_google_chrome"> please either update the CL description to say "spellchecking via a remote service now works in Chromium" or revert this hunk (consider doing this separately either way, IMO) https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... File chrome/browser/resources/options/language_options.html (right): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... chrome/browser/resources/options/language_options.html:53: <span id="spellcheck-language-message" hidden> <span id="spellcheck-language-message" hidden></span> https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:53: * @const {string} try to minimize the amount of changes you make in large CLs... it's just going to make it harder on you, me, higher chance of revert, etc. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:630: updateSpellCheckLanguageControls_: function(languageCode) { assert(languageCode); https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:995: // e.value.value can be an empty string. If it is, then splitting it will "e.value.value can be an empty string." -> why? https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:1004: this.spellCheckLanguages_[languages[i]] = true; wrong indent https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:1442: instance.uiLanguageSaved_(languageCode); i meant `var instance` only when you need to wrap https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... File chrome/browser/resources/options/options.js (left): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... chrome/browser/resources/options/options.js:98: // 'spelling-enabled-control' element is only present on Chrome branded split to separate change, IMO https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... File chrome/browser/resources/options/options.js (right): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... chrome/browser/resources/options/options.js:242: // is enabled so the option will not be shown. why is this check in this file? it should be in browser_options.js, I'd think https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... chrome/browser/resources/options/options.js:245: } can this be $('spelling-enabled-container').hidden = loadTimeData.getBoolean('enableMultilingualSpellChecker'); ? if not, drop the curlies
https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.cc (right): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.cc:23: ""); nit: std::string() instead of "" https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js (right): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:18: * Browse to the language options page & call our preLoad(). & -> and eh, this whole doc comment should probably just be: /** @override */ https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:26: accessibilityIssuesAreErrors: true, reduce yo copy pasta /** @return {Array<string>} The currently selected languages. */ getLanguages: function() { return Object.keys(LanguageOptions.getInstance().spellCheckLanguages_); }, https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:38: var langs = Object.keys(langOpts.spellCheckLanguages_); make your variables as verbose as possible without adding more lines, IMO for example, this could be selectedLanguages or languages without adding more lines, as far as I can tell. apply to everything here (e.g. opts, reg, maybe not "prefs") https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:139: /** @constructor @extends {...} */ like above https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:160: } why is this in the middle of MultilanguagePreferenceWebUIBrowserTest and MultilanguagePreferenceWebUIBrowserTest.prototype? https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:172: /** @override */ https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:188: expectTrue($('spellcheck-language-button').hidden); this class looks pretty similar to MultilanguageOptionsWebUIBrowserTest though I don't often encourage concrete inheritance, you could probably save some code in this case https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:218: waitForResponse(langOpts, 'updateSpellCheckLanguageControls_', I don't suppose you could just do: options.Preferences.getInstance().addEventListener('pref', function() { // things will already be updated because other listeners were subscribed // earlier and already ran testDone(); }); instead of the waitForResponse() stuff you're copying from options_browsertest.js?
Patchset #17 (id:800001) has been deleted
Patchset #17 (id:820001) has been deleted
Patchset #18 (id:860001) has been deleted
Patchset #17 (id:840001) has been deleted
Patchset #17 (id:880001) has been deleted
Patchset #17 (id:900001) has been deleted
Patchset #17 (id:920001) has been deleted
Patchset #17 (id:940001) has been deleted
Rouslan, PTAL at Patch Set #17. This is a merger of the big CL and all of the smaller ones. I also replied to all the old comments. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... File chrome/browser/resources/options/browser_options.html (right): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... chrome/browser/resources/options/browser_options.html:419: class="checkbox controlled-setting-with-label"> On 2015/06/25 02:25:59, Dan Beam wrote: > 4 \s indent Done. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... chrome/browser/resources/options/browser_options.html:434: <if expr="_google_chrome"> On 2015/06/25 02:25:59, Dan Beam wrote: > this should probably be above <div id="metrics-reporting-setting"> Done. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... chrome/browser/resources/options/browser_options.html:434: <if expr="_google_chrome"> On 2015/06/25 02:25:59, Dan Beam wrote: > please either update the CL description to say "spellchecking via a remote > service now works in Chromium" or revert this hunk (consider doing this > separately either way, IMO) Did it in a new CL. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... File chrome/browser/resources/options/language_options.html (right): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... chrome/browser/resources/options/language_options.html:53: <span id="spellcheck-language-message" hidden> On 2015/06/25 02:25:59, Dan Beam wrote: > <span id="spellcheck-language-message" hidden></span> Done. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:53: * @const {string} On 2015/06/25 02:25:59, Dan Beam wrote: > try to minimize the amount of changes you make in large CLs... it's just going > to make it harder on you, me, higher chance of revert, etc. I'll undo these @const changes. They're unnecessary and all other compiler directives in this file are old anyway. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:630: updateSpellCheckLanguageControls_: function(languageCode) { On 2015/06/25 02:25:59, Dan Beam wrote: > assert(languageCode); Done. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:995: // e.value.value can be an empty string. If it is, then splitting it will On 2015/06/25 02:26:00, Dan Beam wrote: > "e.value.value can be an empty string." -> why? Since we're using a ListPref now this doesn't apply. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:1004: this.spellCheckLanguages_[languages[i]] = true; On 2015/06/25 02:26:00, Dan Beam wrote: > wrong indent Done. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:1442: instance.uiLanguageSaved_(languageCode); On 2015/06/25 02:26:00, Dan Beam wrote: > i meant `var instance` only when you need to wrap Done. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... File chrome/browser/resources/options/options.js (left): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... chrome/browser/resources/options/options.js:98: // 'spelling-enabled-control' element is only present on Chrome branded On 2015/06/25 02:26:00, Dan Beam wrote: > split to separate change, IMO Yup, submitted in separate CL. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... File chrome/browser/resources/options/options.js (right): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... chrome/browser/resources/options/options.js:242: // is enabled so the option will not be shown. On 2015/06/25 02:26:00, Dan Beam wrote: > why is this check in this file? it should be in browser_options.js, I'd think Done. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resourc... chrome/browser/resources/options/options.js:245: } On 2015/06/25 02:26:00, Dan Beam wrote: > can this be > > $('spelling-enabled-container').hidden = > loadTimeData.getBoolean('enableMultilingualSpellChecker'); > > ? > > if not, drop the curlies Done. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_factory.cc (right): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_factory.cc:75: chrome::spellcheck_common::IsMultilingualSpellcheckEnabled() On 2015/06/24 22:12:23, groby_OOO_till_7_20 wrote: > Hrmpfh. That means we're leaving an old pref lying around. I'd rather we migrate > dictionaries to a list, independent of IsMultilingual. (I'm not sure we _can_ > migrate types for preferences, though. Which would be sad...) > > I'd also, orthogonal to _that_ decision, like the multi-dictionary method to be > a ListPref Done and Done! https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.cc (left): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:108: if (spellcheck_mac::SpellCheckerAvailable()) On 2015/06/24 22:12:23, groby_OOO_till_7_20 wrote: > Why are we getting rid of the OSX path? This function never actually gets called on OSX. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:49: std::vector<std::string> dictionary_languages; On 2015/06/24 22:12:23, groby_OOO_till_7_20 wrote: > As said above, please use a list pref. Preferences have built-in support for > lists, we shouldn't hand-code our own formats on top of preferences if we don't > have to. Done. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:115: base::SupportsUserData* context, On 2015/06/24 22:12:23, groby_OOO_till_7_20 wrote: > Why move this to SupportsUserData? This makes it easier to test that GetSpellCheckLanguages is working properly. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:288: if (chrome::spellcheck_common::IsMultilingualSpellcheckEnabled()) { On 2015/06/24 22:12:23, groby_OOO_till_7_20 wrote: > If we migrated prefs, that would turn into a single code path... Acknowledged. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service_unittest.cc (right): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service_unittest.cc:23: struct SpellcheckLanguageTestCase { On 2015/06/24 22:12:23, groby_OOO_till_7_20 wrote: > FWIW, you're both adding new test cases and turning the old test cases into a > TEST_CASE_P - you might want to factor out the latter and do that in a separate > CL. It'll make reviewing and understanding changes easier. Done. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/spellch... File chrome/browser/spellchecker/spelling_service_client.cc (right): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/spellch... chrome/browser/spellchecker/spelling_service_client.cc:119: // multilingual spellchecking is enabled the spelling service should be On 2015/06/24 22:12:23, groby_OOO_till_7_20 wrote: > Why do we not want the spelling service for multilingual checking? I believe we've talked about this (spelling server isn't built for multiple languages, etc.) https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.cc (right): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.cc:23: ""); On 2015/06/25 02:44:27, Dan Beam wrote: > nit: std::string() instead of "" Done. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js (right): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:18: * Browse to the language options page & call our preLoad(). On 2015/06/25 02:44:28, Dan Beam wrote: > & -> and > > eh, this whole doc comment should probably just be: > > /** @override */ Done. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:26: accessibilityIssuesAreErrors: true, On 2015/06/25 02:44:28, Dan Beam wrote: > reduce yo copy pasta > > /** @return {Array<string>} The currently selected languages. */ > getLanguages: function() { > return Object.keys(LanguageOptions.getInstance().spellCheckLanguages_); > }, Done. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:38: var langs = Object.keys(langOpts.spellCheckLanguages_); On 2015/06/25 02:44:27, Dan Beam wrote: > make your variables as verbose as possible without adding more lines, IMO > > for example, this could be selectedLanguages or languages without adding more > lines, as far as I can tell. apply to everything here (e.g. opts, reg, maybe > not "prefs") Done. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:139: On 2015/06/25 02:44:27, Dan Beam wrote: > /** @constructor @extends {...} */ like above Deleted this. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:160: } On 2015/06/25 02:44:28, Dan Beam wrote: > why is this in the middle of MultilanguagePreferenceWebUIBrowserTest and > MultilanguagePreferenceWebUIBrowserTest.prototype? Deleted this. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:172: On 2015/06/25 02:44:27, Dan Beam wrote: > /** @override */ Done. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:188: expectTrue($('spellcheck-language-button').hidden); On 2015/06/25 02:44:27, Dan Beam wrote: > this class looks pretty similar to MultilanguageOptionsWebUIBrowserTest > > though I don't often encourage concrete inheritance, you could probably save > some code in this case Done. https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:218: waitForResponse(langOpts, 'updateSpellCheckLanguageControls_', On 2015/06/25 02:44:28, Dan Beam wrote: > I don't suppose you could just do: > > options.Preferences.getInstance().addEventListener('pref', function() { > // things will already be updated because other listeners were subscribed > // earlier and already ran > testDone(); > }); > > instead of the waitForResponse() stuff you're copying from > options_browsertest.js? Done.
https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:13: #include "base/strings/string_split.h" Unused include. https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:14: #include "base/strings/string_util.h" Unused include. https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.cc (left): https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:130: #if defined(OS_MACOSX) If you're removing Mac specific code because this function is not called on Mac, please put the whole function into "#if !defined(OS_MACOSX)" block. https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:309: prefs->GetList(prefs::kSpellCheckDictionaries)->GetString(0, &dictionary); The current implementation is fine as is. Why change it? https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.h (right): https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.h:145: // Reacts to a change in user preference on which language should be used for s/language/languages https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/language_options_handler_common.cc (left): https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/language_options_handler_common.cc:257: CHECK(!language_code.empty()); Why remove this? https://codereview.chromium.org/1156473007/diff/960001/chrome/common/spellche... File chrome/common/spellcheck_common.cc (right): https://codereview.chromium.org/1156473007/diff/960001/chrome/common/spellche... chrome/common/spellcheck_common.cc:189: std::vector<std::string> GetDictionaryLanguagesPref(PrefService* prefs) { Now that we are not storing the list in string, this function is not necessary. I recommend that you remove it and get the preference manually.
https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:13: #include "base/strings/string_split.h" Unused include. https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:14: #include "base/strings/string_util.h" Unused include. https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.cc (left): https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:130: #if defined(OS_MACOSX) If you're removing Mac specific code because this function is not called on Mac, please put the whole function into "#if !defined(OS_MACOSX)" block. https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:309: prefs->GetList(prefs::kSpellCheckDictionaries)->GetString(0, &dictionary); The current implementation is fine as is. Why change it? https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.h (right): https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.h:145: // Reacts to a change in user preference on which language should be used for s/language/languages https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/language_options_handler_common.cc (left): https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/language_options_handler_common.cc:257: CHECK(!language_code.empty()); Why remove this? https://codereview.chromium.org/1156473007/diff/960001/chrome/common/spellche... File chrome/common/spellcheck_common.cc (right): https://codereview.chromium.org/1156473007/diff/960001/chrome/common/spellche... chrome/common/spellcheck_common.cc:189: std::vector<std::string> GetDictionaryLanguagesPref(PrefService* prefs) { Now that we are not storing the list in string, this function is not necessary. I recommend that you remove it and get the preference manually.
https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/resourc... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:616: $('spellcheck-language-button'); fits on one line https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:639: if (languageCode in loadTimeData.getValue('spellCheckLanguageCodeSet')) { if (!(languageCode in loadTimeData.getValue('spellCheckLanguageCodeSet'))) { spellCheckLanguageMessage.textContent = loadTimeData.getString('cannotBeUsedForSpellChecking'); spellCheckLanguageMessage.hidden = false; return; } https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:643: this.spellCheckLanguages_.hasOwnProperty(languageCode); use "key in obj" or "obj.hasOwnProperty(key)", don't mix them https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:646: if (!(languageCode in this.spellcheckDictionaryDownloadStatus_)) { nit: this code would really benefit from things like: var tempVariable = languageCode in this.oneOfManyDictionaries_; where |tempVariable| is something like |isDownloading| or whatever the hell all these if()'s mean https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:969: this.spellCheckLanguages_[languages[i]] = true; curlies https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:1011: var languageCode = e.target.languageCode; target -> currentTarget https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js (right): https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:41: var languages = Object.keys(languageOptions.spellCheckLanguages_); use this.getLanguages() https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:67: var languages = this.getLanguages(); you don't use this... you only reset it later... https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:85: expectEquals(2, languages.length); alternatively just call .getLanguages().length and .getLanguages()[0] if you never want the result to be cached (I don't care if it's .1ms slower, it's test code) https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:112: var languages = this.getLanguages(); put this right before you actually use |languages| https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:127: var languages = this.getLanguages(); lower and/or .getLanguages() every time https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:169: var langs = Object.keys(LanguageOptions.getInstance().spellCheckLanguages_); ... https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:179: var languages = this.getLanguages(); unused?
Patchset #18 (id:980001) has been deleted
Rouslan, PTAL @ Patch Set #18, files: chrome/browser/spellchecker/spellcheck_service.h chrome/browser/spellchecker/spellcheck_service.cc chrome/browser/spellchecker/spellcheck_service_unittest.cc chrome/common/spellcheck_common.cc chrome/common/spellcheck_common.h https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/rendere... File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:13: #include "base/strings/string_split.h" On 2015/07/06 23:28:15, Rouslan wrote: > Unused include. Done. https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/rendere... chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:14: #include "base/strings/string_util.h" On 2015/07/06 23:28:15, Rouslan wrote: > Unused include. Done. https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/resourc... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:616: $('spellcheck-language-button'); On 2015/07/06 23:30:28, Dan Beam wrote: > fits on one line Done. https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:639: if (languageCode in loadTimeData.getValue('spellCheckLanguageCodeSet')) { On 2015/07/06 23:30:28, Dan Beam wrote: > if (!(languageCode in loadTimeData.getValue('spellCheckLanguageCodeSet'))) { > spellCheckLanguageMessage.textContent = > loadTimeData.getString('cannotBeUsedForSpellChecking'); > spellCheckLanguageMessage.hidden = false; > return; > } Done. https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:643: this.spellCheckLanguages_.hasOwnProperty(languageCode); On 2015/07/06 23:30:28, Dan Beam wrote: > use "key in obj" or "obj.hasOwnProperty(key)", don't mix them I'll use "key in obj" because that's what's used throughout the file. https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:646: if (!(languageCode in this.spellcheckDictionaryDownloadStatus_)) { On 2015/07/06 23:30:28, Dan Beam wrote: > nit: this code would really benefit from things like: > > var tempVariable = languageCode in this.oneOfManyDictionaries_; > > where |tempVariable| is something like |isDownloading| or whatever the hell all > these if()'s mean Done. https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:969: this.spellCheckLanguages_[languages[i]] = true; On 2015/07/06 23:30:28, Dan Beam wrote: > curlies Done. https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/resourc... chrome/browser/resources/options/language_options.js:1011: var languageCode = e.target.languageCode; On 2015/07/06 23:30:28, Dan Beam wrote: > target -> currentTarget Done. https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.cc (left): https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:130: #if defined(OS_MACOSX) On 2015/07/06 23:28:15, Rouslan wrote: > If you're removing Mac specific code because this function is not called on Mac, > please put the whole function into "#if !defined(OS_MACOSX)" block. Done. https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.cc:309: prefs->GetList(prefs::kSpellCheckDictionaries)->GetString(0, &dictionary); On 2015/07/06 23:28:15, Rouslan wrote: > The current implementation is fine as is. Why change it? Probably the better way. https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/spellch... File chrome/browser/spellchecker/spellcheck_service.h (right): https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/spellch... chrome/browser/spellchecker/spellcheck_service.h:145: // Reacts to a change in user preference on which language should be used for On 2015/07/06 23:28:15, Rouslan wrote: > s/language/languages Done. https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/language_options_handler_common.cc (left): https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/language_options_handler_common.cc:257: CHECK(!language_code.empty()); On 2015/07/06 23:28:15, Rouslan wrote: > Why remove this? Because now language_code can actually be empty if the user unselects all languages. https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/ui/webu... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js (right): https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:41: var languages = Object.keys(languageOptions.spellCheckLanguages_); On 2015/07/06 23:30:28, Dan Beam wrote: > use this.getLanguages() Done. https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:67: var languages = this.getLanguages(); On 2015/07/06 23:30:28, Dan Beam wrote: > you don't use this... you only reset it later... I was thinking to declare variables at the start of the test. I'll move the declaration down to where it's used. https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:85: expectEquals(2, languages.length); On 2015/07/06 23:30:28, Dan Beam wrote: > alternatively just call .getLanguages().length and .getLanguages()[0] if you > never want the result to be cached (I don't care if it's .1ms slower, it's test > code) I think it's a bit more clear using languages instead of this.getLanguages() every time. https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:112: var languages = this.getLanguages(); On 2015/07/06 23:30:28, Dan Beam wrote: > put this right before you actually use |languages| Done. https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:127: var languages = this.getLanguages(); On 2015/07/06 23:30:28, Dan Beam wrote: > lower and/or .getLanguages() every time Done. https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:169: var langs = Object.keys(LanguageOptions.getInstance().spellCheckLanguages_); On 2015/07/06 23:30:28, Dan Beam wrote: > ... Done. https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/ui/webu... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:179: var languages = this.getLanguages(); On 2015/07/06 23:30:28, Dan Beam wrote: > unused? Moved it down into the event listener callback. https://codereview.chromium.org/1156473007/diff/960001/chrome/common/spellche... File chrome/common/spellcheck_common.cc (right): https://codereview.chromium.org/1156473007/diff/960001/chrome/common/spellche... chrome/common/spellcheck_common.cc:189: std::vector<std::string> GetDictionaryLanguagesPref(PrefService* prefs) { On 2015/07/06 23:28:15, Rouslan wrote: > Now that we are not storing the list in string, this function is not necessary. > I recommend that you remove it and get the preference manually. Done.
Dan, PTAL @ Patch Set #18, files: chrome/browser/resources/options/language_options.html chrome/browser/resources/options/language_options.js chrome/browser/ui/webui/options/language_options_dictionary_download_browsertest.js chrome/browser/ui/webui/options/language_options_handler_common.cc chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.h chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.cc chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js
Just a few clean ups. https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/spellc... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/spellc... chrome/browser/spellchecker/spellcheck_service.cc:52: } Unnecessary whitespace and curly changes. Please undo. https://codereview.chromium.org/1156473007/diff/1000001/chrome/common/spellch... File chrome/common/spellcheck_common.cc (right): https://codereview.chromium.org/1156473007/diff/1000001/chrome/common/spellch... chrome/common/spellcheck_common.cc:11: #include "base/prefs/pref_member.h" Unused. Remove. https://codereview.chromium.org/1156473007/diff/1000001/chrome/common/spellch... chrome/common/spellcheck_common.cc:12: #include "base/prefs/pref_service.h" Unused. Remove. https://codereview.chromium.org/1156473007/diff/1000001/chrome/common/spellch... chrome/common/spellcheck_common.cc:14: #include "chrome/common/pref_names.h" Unused. Remove. https://codereview.chromium.org/1156473007/diff/1000001/chrome/common/spellch... File chrome/common/spellcheck_common.h (right): https://codereview.chromium.org/1156473007/diff/1000001/chrome/common/spellch... chrome/common/spellcheck_common.h:11: class PrefService; Unused. Remove.
juliusa@google.com changed reviewers: + avi@chromium.org
avi@chromium.org: PTAL at Patch Set #19, files: chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/spellc... File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/spellc... chrome/browser/spellchecker/spellcheck_service.cc:52: } On 2015/07/07 01:30:40, Rouslan wrote: > Unnecessary whitespace and curly changes. Please undo. Done. https://codereview.chromium.org/1156473007/diff/1000001/chrome/common/spellch... File chrome/common/spellcheck_common.cc (right): https://codereview.chromium.org/1156473007/diff/1000001/chrome/common/spellch... chrome/common/spellcheck_common.cc:11: #include "base/prefs/pref_member.h" On 2015/07/07 01:30:40, Rouslan wrote: > Unused. Remove. Done. https://codereview.chromium.org/1156473007/diff/1000001/chrome/common/spellch... chrome/common/spellcheck_common.cc:12: #include "base/prefs/pref_service.h" On 2015/07/07 01:30:40, Rouslan wrote: > Unused. Remove. Done. https://codereview.chromium.org/1156473007/diff/1000001/chrome/common/spellch... chrome/common/spellcheck_common.cc:14: #include "chrome/common/pref_names.h" On 2015/07/07 01:30:40, Rouslan wrote: > Unused. Remove. Done. https://codereview.chromium.org/1156473007/diff/1000001/chrome/common/spellch... File chrome/common/spellcheck_common.h (right): https://codereview.chromium.org/1156473007/diff/1000001/chrome/common/spellch... chrome/common/spellcheck_common.h:11: class PrefService; On 2015/07/07 01:30:40, Rouslan wrote: > Unused. Remove. Done.
Rouslan, PTAL @ Patch Set #19, files: chrome/browser/spellchecker/spellcheck_service.h chrome/browser/spellchecker/spellcheck_service.cc chrome/browser/spellchecker/spellcheck_service_unittest.cc chrome/common/spellcheck_common.cc chrome/common/spellcheck_common.h
chrome/browser/spellchecker/* lgtm
https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/resour... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/resour... chrome/browser/resources/options/language_options.js:636: var isLanguageCodeInCodeSet = what does "InCodeSet" mean? https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/resour... chrome/browser/resources/options/language_options.js:647: } else { no } else { after return https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/resour... chrome/browser/resources/options/language_options.js:1434: }; \n https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/ui/web... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js (right): https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:40: var languageOptions = LanguageOptions.getInstance(); remove languageOptions https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:101: // Make sure es' is the only language being spellchecked with. es' -> 'es'
Dan, PTAL @ Patch Set #20, files: chrome/browser/resources/options/language_options.js chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/resour... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/resour... chrome/browser/resources/options/language_options.js:636: var isLanguageCodeInCodeSet = On 2015/07/08 01:13:10, Dan Beam wrote: > what does "InCodeSet" mean? The language code set is the collection of all languages that are allowed to be spellchecked with. So if you look at your languages in your settings menu one might be English which is 'en'. That can't be spellchecked with so its code isn't in the code set. But the different English dialects are in the code set and can be spellchecked with, such as 'en-AU'. This is basically just for if the language cannot be used for spellchecking. https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/resour... chrome/browser/resources/options/language_options.js:647: } else { On 2015/07/08 01:13:10, Dan Beam wrote: > no } else { after return Done. https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/resour... chrome/browser/resources/options/language_options.js:1434: }; On 2015/07/08 01:13:10, Dan Beam wrote: > \n Done. https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/ui/web... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js (right): https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:40: var languageOptions = LanguageOptions.getInstance(); On 2015/07/08 01:13:10, Dan Beam wrote: > remove languageOptions Done. https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:101: // Make sure es' is the only language being spellchecked with. On 2015/07/08 01:13:10, Dan Beam wrote: > es' -> 'es' Done.
https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/resour... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/resour... chrome/browser/resources/options/language_options.js:636: var isLanguageCodeInCodeSet = On 2015/07/08 01:33:46, juliusa wrote: > On 2015/07/08 01:13:10, Dan Beam wrote: > > what does "InCodeSet" mean? > > The language code set is the collection of all languages that are allowed to be > spellchecked with. So if you look at your languages in your settings menu one > might be English which is 'en'. That can't be spellchecked with so its code > isn't in the code set. But the different English dialects are in the code set > and can be spellchecked with, such as 'en-AU'. > > This is basically just for if the language cannot be used for spellchecking. then why not |canBeUsedForSpellChecking| instead? rinse+repeat for other vars
https://codereview.chromium.org/1156473007/diff/1040001/chrome/browser/resour... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/1040001/chrome/browser/resour... chrome/browser/resources/options/language_options.js:638: var isLanguageCodeInLanguages = languageCode in this.spellCheckLanguages_; better name (still have no idea what this means) https://codereview.chromium.org/1156473007/diff/1040001/chrome/browser/resour... chrome/browser/resources/options/language_options.js:640: !(languageCode in this.spellcheckDictionaryDownloadStatus_); move these below if (!) { return } block https://codereview.chromium.org/1156473007/diff/1040001/chrome/browser/resour... chrome/browser/resources/options/language_options.js:640: !(languageCode in this.spellcheckDictionaryDownloadStatus_); isLanguageDownloaded? https://codereview.chromium.org/1156473007/diff/1040001/chrome/browser/ui/web... File chrome/browser/ui/webui/options/language_options_handler_common.cc (left): https://codereview.chromium.org/1156473007/diff/1040001/chrome/browser/ui/web... chrome/browser/ui/webui/options/language_options_handler_common.cc:257: CHECK(!language_code.empty()); why are you removing this? https://codereview.chromium.org/1156473007/diff/1040001/chrome/browser/ui/web... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js (right): https://codereview.chromium.org/1156473007/diff/1040001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:28: return Object.keys(LanguageOptions.getInstance().spellCheckLanguages_); all you ever do is just check the size/contents of getLanguages(). I think this would simplify your code: /** @param {string} Sorted, expected currently selected languages. */ expectCurrentlySelected: function(expected) { var languages = LanguageOptions.getInstance().spellCheckLanguages_; expectEquals(expected, Object.keys(languageOptions).sort().join()); } then later: this.expectCurrentlySelected('fr'); this.expectCurrentlySelected('es,fr'); this.expectCurrentlySelected(''); and so forth
Patchset #21 (id:1060001) has been deleted
Dan, PTAL @ Patch Set #21, files: chrome/browser/resources/options/language_options.js chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/resour... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/resour... chrome/browser/resources/options/language_options.js:636: var isLanguageCodeInCodeSet = On 2015/07/08 18:41:52, Dan Beam wrote: > On 2015/07/08 01:33:46, juliusa wrote: > > On 2015/07/08 01:13:10, Dan Beam wrote: > > > what does "InCodeSet" mean? > > > > The language code set is the collection of all languages that are allowed to > be > > spellchecked with. So if you look at your languages in your settings menu one > > might be English which is 'en'. That can't be spellchecked with so its code > > isn't in the code set. But the different English dialects are in the code set > > and can be spellchecked with, such as 'en-AU'. > > > > This is basically just for if the language cannot be used for spellchecking. > > then why not |canBeUsedForSpellChecking| instead? > > rinse+repeat for other vars isLanguageCodeInCodeSet -> canBeUsedForSpellchecking isLanguageCodeInLanguages -> isUsedForSpellchecking isLanguageUsedForSpellchecking -> isLanguageDownloaded https://codereview.chromium.org/1156473007/diff/1040001/chrome/browser/resour... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/1040001/chrome/browser/resour... chrome/browser/resources/options/language_options.js:638: var isLanguageCodeInLanguages = languageCode in this.spellCheckLanguages_; On 2015/07/08 18:56:27, Dan Beam wrote: > better name (still have no idea what this means) isLanguageCodeInLanguages -> isUsedForSpellchecking https://codereview.chromium.org/1156473007/diff/1040001/chrome/browser/resour... chrome/browser/resources/options/language_options.js:640: !(languageCode in this.spellcheckDictionaryDownloadStatus_); On 2015/07/08 18:56:27, Dan Beam wrote: > move these below if (!) { return } block Done. https://codereview.chromium.org/1156473007/diff/1040001/chrome/browser/resour... chrome/browser/resources/options/language_options.js:640: !(languageCode in this.spellcheckDictionaryDownloadStatus_); On 2015/07/08 18:56:27, Dan Beam wrote: > isLanguageDownloaded? Done. https://codereview.chromium.org/1156473007/diff/1040001/chrome/browser/ui/web... File chrome/browser/ui/webui/options/language_options_handler_common.cc (left): https://codereview.chromium.org/1156473007/diff/1040001/chrome/browser/ui/web... chrome/browser/ui/webui/options/language_options_handler_common.cc:257: CHECK(!language_code.empty()); On 2015/07/08 18:56:27, Dan Beam wrote: > why are you removing this? language_code can actually be empty now because in multilingual spellchecking the user can deselect all languages. https://codereview.chromium.org/1156473007/diff/1040001/chrome/browser/ui/web... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js (right): https://codereview.chromium.org/1156473007/diff/1040001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:28: return Object.keys(LanguageOptions.getInstance().spellCheckLanguages_); On 2015/07/08 18:56:27, Dan Beam wrote: > all you ever do is just check the size/contents of getLanguages(). I think this > would simplify your code: > > /** @param {string} Sorted, expected currently selected languages. */ > expectCurrentlySelected: function(expected) { > var languages = LanguageOptions.getInstance().spellCheckLanguages_; > expectEquals(expected, Object.keys(languageOptions).sort().join()); > } > > then later: > > this.expectCurrentlySelected('fr'); > this.expectCurrentlySelected('es,fr'); > this.expectCurrentlySelected(''); > > and so forth Done.
https://codereview.chromium.org/1156473007/diff/1040001/chrome/browser/ui/web... File chrome/browser/ui/webui/options/language_options_handler_common.cc (left): https://codereview.chromium.org/1156473007/diff/1040001/chrome/browser/ui/web... chrome/browser/ui/webui/options/language_options_handler_common.cc:257: CHECK(!language_code.empty()); On 2015/07/08 20:38:48, juliusa wrote: > On 2015/07/08 18:56:27, Dan Beam wrote: > > why are you removing this? > > language_code can actually be empty now because in multilingual spellchecking > the user can deselect all languages. why can they de-select all languages?
On 2015/07/08 22:50:24, Dan Beam wrote: > why can they de-select all languages? Short answer: To switch between two languages, a user might first deselect the current language, then select the desired language instead. Long answer: This particular point was not explicitly addressed in the UI review, but they approved the overall design. We showed them mocks from the design doc: https://docs.google.com/document/d/1-K47_jxoMUStrFEOYZS4L7S60z0YxptfUPfEySNv1... Their only request was to change "Use this language for spellchecker" in settings from buttons to checkboxes. This was over VC and I cannot find the relevant emails, but here is me requested Tyler for a UX review here: https://code.google.com/p/chromium/issues/detail?id=5102#c139 And here's me getting back to the bug report with the suggestions from the UI review: https://code.google.com/p/chromium/issues/detail?id=5102#c142 That being said, I think that logically a user that wants to switch from language A to language B has two paths in multilingual world. 1) First select B, then deselect A. Note that the user will have two spellcheck languages enabled for a short period of time. 2) First deselect A, then select B. Note that the user will have zero spellcheck languages enabled for a short period of time.
On 2015/07/08 23:23:46, Rouslan wrote: > On 2015/07/08 22:50:24, Dan Beam wrote: > > why can they de-select all languages? > > Short answer: To switch between two languages, a user might first deselect the > current language, then select the desired language instead. > > Long answer: This particular point was not explicitly addressed in the UI > review, but they approved the overall design. We showed them mocks from the > design doc: > > https://docs.google.com/document/d/1-K47_jxoMUStrFEOYZS4L7S60z0YxptfUPfEySNv1... > > Their only request was to change "Use this language for spellchecker" in > settings from buttons to checkboxes. This was over VC and I cannot find the > relevant emails, but here is me requested Tyler for a UX review here: > > https://code.google.com/p/chromium/issues/detail?id=5102#c139 > > And here's me getting back to the bug report with the suggestions from the UI > review: > > https://code.google.com/p/chromium/issues/detail?id=5102#c142 > > That being said, I think that logically a user that wants to switch from > language A to language B has two paths in multilingual world. > > 1) First select B, then deselect A. Note that the user will have two spellcheck > languages enabled for a short period of time. > > 2) First deselect A, then select B. Note that the user will have zero spellcheck > languages enabled for a short period of time. has anybody consulted the designers to attempt to find a better UI paradigm? (because this one seems kinda error prone...) I guess there's probably a class of users that doesn't want spellcheck to begin with, and this would satisfy them, so OK. https://codereview.chromium.org/1156473007/diff/1080001/chrome/browser/resour... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/1080001/chrome/browser/resour... chrome/browser/resources/options/language_options.js:192: why did you add this \n? https://codereview.chromium.org/1156473007/diff/1080001/chrome/browser/ui/web... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js (right): https://codereview.chromium.org/1156473007/diff/1080001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:21: typedefCppFixture: 'MultilanguageOptionsWebUIBrowserTest', the C++ and JS class should not be the same name https://codereview.chromium.org/1156473007/diff/1080001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:79: // Make sure 'fr' and 'es' are the only two languages being spellchecked with. don't think these comments are necessary any longer... the code basically spells out the same thing https://codereview.chromium.org/1156473007/diff/1080001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:158: registeredPrefs['spellcheck.dictionaries'].orig.value.length); combine this and the code at L177 https://codereview.chromium.org/1156473007/diff/1080001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:172: options.Preferences.getInstance().addEventListener('spellcheck.dictionaries', var prefs = options.Preferences.getInstance(); prefs.getInstance().addEventListener('spellcheck.dictionaries', https://codereview.chromium.org/1156473007/diff/1080001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:179: registeredPrefs['spellcheck.dictionaries'].orig.value.length); var registerPrefs = prefs.registeredPreferences_; https://codereview.chromium.org/1156473007/diff/1080001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:183: 'fr'); after you bind(), this should work: this.expectCurrentlySelected('fr'); https://codereview.chromium.org/1156473007/diff/1080001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:185: }); }.bind(this)); https://codereview.chromium.org/1156473007/diff/1080001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:189: remove \n
Patchset #22 (id:1100001) has been deleted
Patchset #22 (id:1120001) has been deleted
Patchset #22 (id:1140001) has been deleted
Dan, PTAL @ Patch Set #22, files: chrome/browser/resources/options/language_options.html chrome/browser/resources/options/language_options.js chrome/browser/ui/webui/options/language_options_dictionary_download_browsertest.js chrome/browser/ui/webui/options/language_options_handler_common.cc chrome/browser/ui/webui/options/multilanguage_options_browsertest.h chrome/browser/ui/webui/options/multilanguage_options_browsertest.cc chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js https://codereview.chromium.org/1156473007/diff/1080001/chrome/browser/resour... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/1080001/chrome/browser/resour... chrome/browser/resources/options/language_options.js:192: On 2015/07/08 23:41:36, Dan Beam wrote: > why did you add this \n? I shouldn't have, deleted. https://codereview.chromium.org/1156473007/diff/1080001/chrome/browser/ui/web... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js (right): https://codereview.chromium.org/1156473007/diff/1080001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:21: typedefCppFixture: 'MultilanguageOptionsWebUIBrowserTest', On 2015/07/08 23:41:36, Dan Beam wrote: > the C++ and JS class should not be the same name Done. https://codereview.chromium.org/1156473007/diff/1080001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:79: // Make sure 'fr' and 'es' are the only two languages being spellchecked with. On 2015/07/08 23:41:37, Dan Beam wrote: > don't think these comments are necessary any longer... the code basically spells > out the same thing Done, and I tried to consolidate and trim other unneeded comments elsewhere. https://codereview.chromium.org/1156473007/diff/1080001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:158: registeredPrefs['spellcheck.dictionaries'].orig.value.length); On 2015/07/08 23:41:36, Dan Beam wrote: > combine this and the code at L177 Done. https://codereview.chromium.org/1156473007/diff/1080001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:172: options.Preferences.getInstance().addEventListener('spellcheck.dictionaries', On 2015/07/08 23:41:36, Dan Beam wrote: > var prefs = options.Preferences.getInstance(); > prefs.getInstance().addEventListener('spellcheck.dictionaries', Done. Even though this is the only places prefs is used now, I think it looks a bit cleaner like this. https://codereview.chromium.org/1156473007/diff/1080001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:179: registeredPrefs['spellcheck.dictionaries'].orig.value.length); On 2015/07/08 23:41:37, Dan Beam wrote: > var registerPrefs = prefs.registeredPreferences_; Done. https://codereview.chromium.org/1156473007/diff/1080001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:183: 'fr'); On 2015/07/08 23:41:36, Dan Beam wrote: > after you bind(), this should work: > > this.expectCurrentlySelected('fr'); Done. https://codereview.chromium.org/1156473007/diff/1080001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:185: }); On 2015/07/08 23:41:36, Dan Beam wrote: > }.bind(this)); Done. https://codereview.chromium.org/1156473007/diff/1080001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:189: On 2015/07/08 23:41:37, Dan Beam wrote: > remove \n Done.
chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc lgtm
lgtm w/nits (address them first) https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/resour... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/resour... chrome/browser/resources/options/language_options.js:676: } congratulations on making this code the most readable it's ever been (seriously, it's always been a problem and this is way better) https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/ui/web... File chrome/browser/ui/webui/options/multilanguage_options_browsertest.cc (right): https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_browsertest.cc:23: base::ListValue()); SetDictionaries(base::ListValue()); https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_browsertest.cc:38: dictionaries); SetDictionariesPref(dictionaries); https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_browsertest.cc:47: } add a private void MultilanguageOptionsBrowserTest::SetDictionariesPref( const base::ListValue& value) { browser()->profile()->GetPrefs()->Set(prefs::kSpellCheckDictionaries, value); } https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/ui/web... File chrome/browser/ui/webui/options/multilanguage_options_browsertest.h (right): https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_browsertest.h:17: ~MultilanguageOptionsBrowserTest() override; arguably put a doc comment for |SetBlankDictionaryPref()| if you can think of something it'd add https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_browsertest.h:18: void SetBlankDictionariesPref(); nit: ClearSpellcheckDictionaries() https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/ui/web... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js (right): https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:102: */ ^ put this doc comment block above like this /** * Tests for multilingual spellcheck preferences set from options. * @constructor * @extends {MultilanguageOptionsWebUIBrowserTest} */ function MultilanguagePreferenceWebUIBrowserTest() {} https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:107: typedefCppFixture: 'MultilanguageOptionsWebUIBrowserTest', remove this https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:113: /** @override */
Patchset #23 (id:1180001) has been deleted
The CQ bit was checked by juliusa@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, rouslan@chromium.org, avi@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1156473007/#ps1200001 (title: "Cleaned nits for submit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1156473007/1200001
Finished replying to comments and submitting to CQ. https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/resour... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/resour... chrome/browser/resources/options/language_options.js:676: } On 2015/07/09 17:56:48, Dan Beam wrote: > congratulations on making this code the most readable it's ever been (seriously, > it's always been a problem and this is way better) Acknowledged. https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/ui/web... File chrome/browser/ui/webui/options/multilanguage_options_browsertest.cc (right): https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_browsertest.cc:23: base::ListValue()); On 2015/07/09 17:56:48, Dan Beam wrote: > SetDictionaries(base::ListValue()); Done. https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_browsertest.cc:38: dictionaries); On 2015/07/09 17:56:48, Dan Beam wrote: > SetDictionariesPref(dictionaries); Done. https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_browsertest.cc:47: } On 2015/07/09 17:56:48, Dan Beam wrote: > add a private > > void MultilanguageOptionsBrowserTest::SetDictionariesPref( > const base::ListValue& value) { > browser()->profile()->GetPrefs()->Set(prefs::kSpellCheckDictionaries, value); > } Done. https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/ui/web... File chrome/browser/ui/webui/options/multilanguage_options_browsertest.h (right): https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_browsertest.h:17: ~MultilanguageOptionsBrowserTest() override; On 2015/07/09 17:56:48, Dan Beam wrote: > arguably put a doc comment for |SetBlankDictionaryPref()| if you can think of > something it'd add I think a small comment saying that it sets the pref to an empty list could clarify a bit. https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_browsertest.h:18: void SetBlankDictionariesPref(); On 2015/07/09 17:56:48, Dan Beam wrote: > nit: ClearSpellcheckDictionaries() Done. https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/ui/web... File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js (right): https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:102: */ On 2015/07/09 17:56:48, Dan Beam wrote: > ^ put this doc comment block above like this > > /** > * Tests for multilingual spellcheck preferences set from options. > * @constructor > * @extends {MultilanguageOptionsWebUIBrowserTest} > */ > function MultilanguagePreferenceWebUIBrowserTest() {} Done. https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:107: typedefCppFixture: 'MultilanguageOptionsWebUIBrowserTest', On 2015/07/09 17:56:48, Dan Beam wrote: > remove this Done. https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/ui/web... chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:113: On 2015/07/09 17:56:48, Dan Beam wrote: > /** @override */ Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #23 (id:1200001) has been deleted
The CQ bit was checked by juliusa@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, isherman@chromium.org, rouslan@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1156473007/#ps1210001 (title: " Cleaned nits for submit.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1156473007/1210001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was unchecked by juliusa@google.com
Patchset #23 (id:1210001) has been deleted
The CQ bit was checked by juliusa@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org, isherman@chromium.org, rouslan@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/1156473007/#ps1230001 (title: "Fixed nits and presubmit warnings.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1156473007/1230001
Message was sent while issue was closed.
Committed patchset #23 (id:1230001)
Message was sent while issue was closed.
Patchset 23 (id:??) landed as https://crrev.com/963f5dcf387298e03aeb7c3654890d1ca28d2ca6 Cr-Commit-Position: refs/heads/master@{#338174} |
