Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(549)

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.

Description

Enables 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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+467 lines, -127 lines) Patch
M chrome/app/generated_resources.grd View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -1 line 0 comments Download
M chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 5 chunks +42 lines, -15 lines 0 comments Download
M chrome/browser/resources/options/language_options.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +10 lines, -5 lines 0 comments Download
M chrome/browser/resources/options/language_options.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 12 chunks +108 lines, -57 lines 0 comments Download
M chrome/browser/spellchecker/spellcheck_service.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +9 lines, -7 lines 0 comments Download
M chrome/browser/spellchecker/spellcheck_service.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 7 chunks +13 lines, -26 lines 0 comments Download
M chrome/browser/spellchecker/spellcheck_service_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 5 chunks +30 lines, -9 lines 0 comments Download
M chrome/browser/ui/webui/options/language_options_dictionary_download_browsertest.js View 1 2 3 4 4 chunks +5 lines, -5 lines 0 comments Download
M chrome/browser/ui/webui/options/language_options_handler_common.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +4 lines, -1 line 0 comments Download
A chrome/browser/ui/webui/options/multilanguage_options_browsertest.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +29 lines, -0 lines 0 comments Download
A chrome/browser/ui/webui/options/multilanguage_options_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +50 lines, -0 lines 0 comments Download
A chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +156 lines, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 3 chunks +6 lines, -0 lines 0 comments Download

Messages

Total messages: 124 (59 generated)
Julius
Replied to previous comments and added browser_tests. Picked up where Klemen Forstneric (klemen.forstneric@gmail.com) left off. ...
5 years, 6 months ago (2015-06-03 21:18:39 UTC) #2
Julius
Please hold for reviewing, still getting used to code review tool.
5 years, 6 months ago (2015-06-03 21:21:59 UTC) #4
Julius
Replied to the previous comments and added a couple browser tests.
5 years, 6 months ago (2015-06-04 18:23:22 UTC) #11
please use gerrit instead
A few initial comments to keep you busy. Let me know if you have questions. ...
5 years, 6 months ago (2015-06-04 19:41:06 UTC) #12
Julius
Addressed previous comments, used newly created MultilingualSpellcheckIsEnabled() function where needed. https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1156473007/diff/100001/chrome/browser/about_flags.cc#newcode2277 ...
5 years, 6 months ago (2015-06-05 03:41:13 UTC) #15
please use gerrit instead
Awesome progress so far! I don't want to bog you down with too many comments, ...
5 years, 6 months ago (2015-06-05 17:50:06 UTC) #16
please use gerrit instead
https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc#newcode54 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): ...
5 years, 6 months ago (2015-06-05 17:51:15 UTC) #17
Julius
Addressed previous comments. https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc#newcode54 chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:54: SpellcheckService::GetSpellCheckLanguages(browser_context, &languages_); On 2015/06/05 17:51:15, Rouslan ...
5 years, 6 months ago (2015-06-05 21:38:34 UTC) #18
please use gerrit instead
Great progress! Keep it up! https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resources/options/language_options.js File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/160001/chrome/browser/resources/options/language_options.js#newcode26 chrome/browser/resources/options/language_options.js:26: * @const {string} On ...
5 years, 6 months ago (2015-06-06 01:42:13 UTC) #19
Dan Beam
https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resources/options/language_options.html File chrome/browser/resources/options/language_options.html (right): https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resources/options/language_options.html#newcode45 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/resources/options/language_options.html#newcode48 chrome/browser/resources/options/language_options.html:48: ...
5 years, 6 months ago (2015-06-09 17:57:47 UTC) #20
Julius
I rebuilt the patch focusing on getting the multilanguage browsertest to work and everything seems ...
5 years, 6 months ago (2015-06-12 20:10:16 UTC) #29
Julius
Updated some Javascript-related things. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resources/options/language_options.html File chrome/browser/resources/options/language_options.html (right): https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resources/options/language_options.html#newcode45 chrome/browser/resources/options/language_options.html:45: id="language-options-spellcheck-language-checkbox-div"> On 2015/06/09 17:57:47, Dan ...
5 years, 6 months ago (2015-06-15 22:47:34 UTC) #30
please use gerrit instead
Awesome testing! https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/1156473007/diff/350001/chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc#newcode60 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/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc#newcode62 chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:62: if (chrome::spellcheck_common::MultilingualSpellcheckIsEnabled()) ...
5 years, 6 months ago (2015-06-16 00:31:18 UTC) #31
Julius
Refactored some old tests and addressed comments. https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resources/options/language_options.js File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/180001/chrome/browser/resources/options/language_options.js#newcode145 chrome/browser/resources/options/language_options.js:145: spellCheckDictionary_: '', ...
5 years, 6 months ago (2015-06-17 00:59:53 UTC) #34
please use gerrit instead
Awesome! So close to the finish line... https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellchecker/spellcheck_factory.cc File chrome/browser/spellchecker/spellcheck_factory.cc (right): https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellchecker/spellcheck_factory.cc#newcode78 chrome/browser/spellchecker/spellcheck_factory.cc:78: user_prefs::PrefRegistrySyncable::SYNCABLE_PREF); prefs::kSpellCheckDictionary ...
5 years, 6 months ago (2015-06-17 17:47:16 UTC) #35
Julius
Addressed comments, cleaned tests, removed unused includes. https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellchecker/spellcheck_factory.cc File chrome/browser/spellchecker/spellcheck_factory.cc (right): https://codereview.chromium.org/1156473007/diff/410001/chrome/browser/spellchecker/spellcheck_factory.cc#newcode78 chrome/browser/spellchecker/spellcheck_factory.cc:78: user_prefs::PrefRegistrySyncable::SYNCABLE_PREF); On ...
5 years, 6 months ago (2015-06-17 20:14:46 UTC) #36
please use gerrit instead
https://codereview.chromium.org/1156473007/diff/430001/chrome/browser/spellchecker/spellcheck_service_browsertest.cc File chrome/browser/spellchecker/spellcheck_service_browsertest.cc (right): https://codereview.chromium.org/1156473007/diff/430001/chrome/browser/spellchecker/spellcheck_service_browsertest.cc#newcode51 chrome/browser/spellchecker/spellcheck_service_browsertest.cc:51: accept_languages(accept_languages), +6 spaces of indent here and the one ...
5 years, 6 months ago (2015-06-17 21:15:07 UTC) #37
Julius
Cleaned up the indentation ugliness and the unnecessary base::StringPiece constructors. https://codereview.chromium.org/1156473007/diff/430001/chrome/browser/spellchecker/spellcheck_service_browsertest.cc File chrome/browser/spellchecker/spellcheck_service_browsertest.cc (right): https://codereview.chromium.org/1156473007/diff/430001/chrome/browser/spellchecker/spellcheck_service_browsertest.cc#newcode51 ...
5 years, 6 months ago (2015-06-17 21:45:39 UTC) #38
please use gerrit instead
lgtm % one problem below. You can send this out to OWNERs now. https://codereview.chromium.org/1156473007/diff/450001/chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.h File ...
5 years, 6 months ago (2015-06-17 22:08:37 UTC) #39
Julius
Picked up where Klemen left off, added some tests, and cleaned older code/
5 years, 6 months ago (2015-06-17 22:32:32 UTC) #41
Julius
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
5 years, 6 months ago (2015-06-17 22:50:32 UTC) #44
Julius
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
5 years, 6 months ago (2015-06-17 22:53:26 UTC) #45
Ilya Sherman
histograms.xml lgtm
5 years, 6 months ago (2015-06-17 23:29:44 UTC) #47
Dan Beam
https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/resources/options/language_options.html File chrome/browser/resources/options/language_options.html (right): https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/resources/options/language_options.html#newcode45 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/resources/options/language_options.html#newcode48 chrome/browser/resources/options/language_options.html:48: id="spellcheck-language-checkbox"> ...
5 years, 6 months ago (2015-06-17 23:42:02 UTC) #48
groby-ooo-7-16
https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h File chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h (right): https://codereview.chromium.org/1156473007/diff/470001/chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h#newcode48 chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h:48: size_t selected_languages_; num_selected_languages, please. (Or something else indicating this ...
5 years, 6 months ago (2015-06-18 00:44:30 UTC) #49
Dan Beam
https://codereview.chromium.org/1156473007/diff/540001/chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js (right): https://codereview.chromium.org/1156473007/diff/540001/chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js#newcode29 chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js:29: setUp: function() { testing.Test.prototype.setUp.call(this);
5 years, 6 months ago (2015-06-19 20:43:42 UTC) #52
please use gerrit instead
Julius, I noticed that unselecting all languages does not stop spellchecker from working. You can ...
5 years, 6 months ago (2015-06-21 23:43:44 UTC) #54
please use gerrit instead
Changes look good! I did not find any bugs when testing this. Just a few ...
5 years, 6 months ago (2015-06-23 00:43:06 UTC) #58
please use gerrit instead
A couple of notes. https://codereview.chromium.org/1156473007/diff/740001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/1156473007/diff/740001/chrome/browser/about_flags.cc#newcode2005 chrome/browser/about_flags.cc:2005: "enable-multilingual-spellchecker", previous entry is clang-formatted. ...
5 years, 6 months ago (2015-06-23 21:56:01 UTC) #62
please use gerrit instead
https://codereview.chromium.org/1156473007/diff/760001/chrome/browser/resources/options/language_options.js File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/760001/chrome/browser/resources/options/language_options.js#newcode999 chrome/browser/resources/options/language_options.js:999: if (languages[i] !== '') In which case would |languages[i]| ...
5 years, 6 months ago (2015-06-24 20:35:10 UTC) #63
Julius
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/webui/options/multilanguage_options_webui_browsertest.h File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.h (right): ...
5 years, 6 months ago (2015-06-24 21:18:00 UTC) #64
Julius
Rachel, PTAL at Patch Set #16, please excuse the last email incorrectly stating Patch Set ...
5 years, 6 months ago (2015-06-24 21:20:10 UTC) #65
Julius
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 ...
5 years, 6 months ago (2015-06-24 21:22:02 UTC) #66
groby-ooo-7-16
Mostly still high-level questions https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/spellchecker/spellcheck_factory.cc File chrome/browser/spellchecker/spellcheck_factory.cc (right): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/spellchecker/spellcheck_factory.cc#newcode75 chrome/browser/spellchecker/spellcheck_factory.cc:75: chrome::spellcheck_common::IsMultilingualSpellcheckEnabled() Hrmpfh. That means we're ...
5 years, 6 months ago (2015-06-24 22:12:24 UTC) #67
Dan Beam
https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resources/options/browser_options.html File chrome/browser/resources/options/browser_options.html (right): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/resources/options/browser_options.html#newcode419 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/resources/options/browser_options.html#newcode434 chrome/browser/resources/options/browser_options.html:434: <if expr="_google_chrome"> ...
5 years, 6 months ago (2015-06-25 02:26:00 UTC) #68
Dan Beam
https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.cc File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.cc (right): https://codereview.chromium.org/1156473007/diff/780001/chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.cc#newcode23 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/webui/options/multilanguage_options_webui_browsertest.js File chrome/browser/ui/webui/options/multilanguage_options_webui_browsertest.js ...
5 years, 6 months ago (2015-06-25 02:44:28 UTC) #69
Julius
Rouslan, PTAL at Patch Set #17. This is a merger of the big CL and ...
5 years, 5 months ago (2015-07-06 22:38:54 UTC) #78
please use gerrit instead
https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc#newcode13 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/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc#newcode14 chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:14: #include "base/strings/string_util.h" Unused ...
5 years, 5 months ago (2015-07-06 23:28:13 UTC) #79
please use gerrit instead
https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc (right): https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc#newcode13 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/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc#newcode14 chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc:14: #include "base/strings/string_util.h" Unused ...
5 years, 5 months ago (2015-07-06 23:28:16 UTC) #80
Dan Beam
https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/resources/options/language_options.js File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/resources/options/language_options.js#newcode616 chrome/browser/resources/options/language_options.js:616: $('spellcheck-language-button'); fits on one line https://codereview.chromium.org/1156473007/diff/960001/chrome/browser/resources/options/language_options.js#newcode639 chrome/browser/resources/options/language_options.js:639: if (languageCode ...
5 years, 5 months ago (2015-07-06 23:30:28 UTC) #81
Julius
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/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc File chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc ...
5 years, 5 months ago (2015-07-07 01:14:21 UTC) #83
Julius
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
5 years, 5 months ago (2015-07-07 01:16:58 UTC) #84
please use gerrit instead
Just a few clean ups. https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/spellchecker/spellcheck_service.cc File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/spellchecker/spellcheck_service.cc#newcode52 chrome/browser/spellchecker/spellcheck_service.cc:52: } Unnecessary whitespace and ...
5 years, 5 months ago (2015-07-07 01:30:41 UTC) #85
Julius
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/spellchecker/spellcheck_service.cc File chrome/browser/spellchecker/spellcheck_service.cc (right): https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/spellchecker/spellcheck_service.cc#newcode52 chrome/browser/spellchecker/spellcheck_service.cc:52: ...
5 years, 5 months ago (2015-07-07 22:16:25 UTC) #87
Julius
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
5 years, 5 months ago (2015-07-07 22:17:44 UTC) #88
please use gerrit instead
chrome/browser/spellchecker/* lgtm
5 years, 5 months ago (2015-07-07 22:28:58 UTC) #89
Dan Beam
https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/resources/options/language_options.js File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/resources/options/language_options.js#newcode636 chrome/browser/resources/options/language_options.js:636: var isLanguageCodeInCodeSet = what does "InCodeSet" mean? https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/resources/options/language_options.js#newcode647 chrome/browser/resources/options/language_options.js:647: ...
5 years, 5 months ago (2015-07-08 01:13:10 UTC) #90
Julius
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/resources/options/language_options.js File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/resources/options/language_options.js#newcode636 chrome/browser/resources/options/language_options.js:636: ...
5 years, 5 months ago (2015-07-08 01:33:46 UTC) #91
Dan Beam
https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/resources/options/language_options.js File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/resources/options/language_options.js#newcode636 chrome/browser/resources/options/language_options.js:636: var isLanguageCodeInCodeSet = On 2015/07/08 01:33:46, juliusa wrote: > ...
5 years, 5 months ago (2015-07-08 18:41:52 UTC) #92
Dan Beam
https://codereview.chromium.org/1156473007/diff/1040001/chrome/browser/resources/options/language_options.js File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/1040001/chrome/browser/resources/options/language_options.js#newcode638 chrome/browser/resources/options/language_options.js:638: var isLanguageCodeInLanguages = languageCode in this.spellCheckLanguages_; better name (still ...
5 years, 5 months ago (2015-07-08 18:56:28 UTC) #93
Julius
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/resources/options/language_options.js File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/1000001/chrome/browser/resources/options/language_options.js#newcode636 chrome/browser/resources/options/language_options.js:636: ...
5 years, 5 months ago (2015-07-08 20:38:48 UTC) #95
Dan Beam
https://codereview.chromium.org/1156473007/diff/1040001/chrome/browser/ui/webui/options/language_options_handler_common.cc File chrome/browser/ui/webui/options/language_options_handler_common.cc (left): https://codereview.chromium.org/1156473007/diff/1040001/chrome/browser/ui/webui/options/language_options_handler_common.cc#oldcode257 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 ...
5 years, 5 months ago (2015-07-08 22:50:24 UTC) #96
please use gerrit instead
On 2015/07/08 22:50:24, Dan Beam wrote: > why can they de-select all languages? Short answer: ...
5 years, 5 months ago (2015-07-08 23:23:46 UTC) #97
Dan Beam
On 2015/07/08 23:23:46, Rouslan wrote: > On 2015/07/08 22:50:24, Dan Beam wrote: > > why ...
5 years, 5 months ago (2015-07-08 23:41:37 UTC) #98
Julius
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/resources/options/language_options.js ...
5 years, 5 months ago (2015-07-09 03:54:52 UTC) #102
Avi (use Gerrit)
chrome/browser/renderer_context_menu/spellchecker_submenu_observer.h chrome/browser/renderer_context_menu/spellchecker_submenu_observer_hunspell.cc lgtm
5 years, 5 months ago (2015-07-09 05:30:59 UTC) #103
Dan Beam
lgtm w/nits (address them first) https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/resources/options/language_options.js File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/resources/options/language_options.js#newcode676 chrome/browser/resources/options/language_options.js:676: } congratulations on making ...
5 years, 5 months ago (2015-07-09 17:56:48 UTC) #104
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1156473007/1200001
5 years, 5 months ago (2015-07-09 18:38:59 UTC) #108
Julius
Finished replying to comments and submitting to CQ. https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/resources/options/language_options.js File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/1156473007/diff/1160001/chrome/browser/resources/options/language_options.js#newcode676 chrome/browser/resources/options/language_options.js:676: } ...
5 years, 5 months ago (2015-07-09 18:49:11 UTC) #109
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/77531)
5 years, 5 months ago (2015-07-09 18:52:43 UTC) #111
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1156473007/1210001
5 years, 5 months ago (2015-07-09 21:12:33 UTC) #115
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/77608)
5 years, 5 months ago (2015-07-09 21:24:07 UTC) #117
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1156473007/1230001
5 years, 5 months ago (2015-07-09 22:02:45 UTC) #122
commit-bot: I haz the power
Committed patchset #23 (id:1230001)
5 years, 5 months ago (2015-07-09 22:51:38 UTC) #123
commit-bot: I haz the power
5 years, 5 months ago (2015-07-09 22:52:39 UTC) #124
Message was sent while issue was closed.
Patchset 23 (id:??) landed as
https://crrev.com/963f5dcf387298e03aeb7c3654890d1ca28d2ca6
Cr-Commit-Position: refs/heads/master@{#338174}

Powered by Google App Engine
This is Rietveld 408576698