Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1283603002:
Implement langageSettingsPrivate API. (Closed)
|
Created:
5 years, 4 months ago by michaelpg Modified:
5 years, 4 months ago Reviewers:
hajimehoshi, stevenjb, please use gerrit instead, jungshik at Google, Seigo Nonaka, Evan Stade CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@LanguageSettingsAPIIDLChanges Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement langageSettingsPrivate API.
This implements language and spellcheck settings, but not the input method
settings.
BUG=479043
R=stevenjb@chromium.org
Committed: https://crrev.com/c8976b127fba81baf84029d0c9bc6ccda2e1c298
Cr-Commit-Position: refs/heads/master@{#344108}
Patch Set 1 : #
Total comments: 14
Patch Set 2 : stevenjb feedback #
Total comments: 15
Patch Set 3 : feedback #
Total comments: 1
Patch Set 4 : std::unordered_set --> base::hash_set #Patch Set 5 : Fix const error in l10n_util_collator #
Total comments: 4
Messages
Total messages: 48 (18 generated)
Patchset #1 (id:1) has been deleted
stevenjb: this is the implementation of languageSettingsPrivate
which follows the previous CL I just sent out for review:
https://codereview.chromium.org/1274753006/
Feel free to let me know if there are glaring issues or
structural changes I should address before a full review.
This generally looks OK to me. You'll definitely want to try to find someone familiar with the original code to take a look also. https://codereview.chromium.org/1283603002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc (right): https://codereview.chromium.org/1283603002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:64: LanguageMap language_map(c); nit: elim |c| https://codereview.chromium.org/1283603002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:67: for (size_t i = 0; i < language_codes.size(); ++i) { for (auto code : language_codes) https://codereview.chromium.org/1283603002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:98: ++it) { for (auto it : language_map) https://codereview.chromium.org/1283603002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:213: TranslateService::GetTargetLanguage(prefs); nit: no need for local var https://codereview.chromium.org/1283603002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/language_settings_private/language_settings_private_delegate.cc (right): https://codereview.chromium.org/1283603002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_delegate.cc:86: listening_spellcheck_ = false; nit: This would be more clear inside the if() body https://codereview.chromium.org/1283603002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/language_settings_private/language_settings_private_delegate.h (right): https://codereview.chromium.org/1283603002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_delegate.h:39: scoped_ptr<api::language_settings_private::SpellcheckDictionaryStatus>> ScopedVector?
Patchset #2 (id:40001) has been deleted
michaelpg@chromium.org changed reviewers: + shuchen@chromium.org
PTAL. Also rebased. https://codereview.chromium.org/1283603002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc (right): https://codereview.chromium.org/1283603002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:64: LanguageMap language_map(c); On 2015/08/10 16:13:54, stevenjb wrote: > nit: elim |c| I don't write enough C++ these days to be confident on this, so please check my logic: 1. it has to be 2 lines anyway; collator.get() is an argument to a regular explicit constructor, not a copy constructor or anything, so the class name is still necessary 2. the StringComparator being passed as a const reference, which std::map then makes and stores a copy of. StringComparator has an implicit, trivial copy constructor, so this is safe -- the temporary StringComparator created here can be destroyed immediately https://codereview.chromium.org/1283603002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:67: for (size_t i = 0; i < language_codes.size(); ++i) { On 2015/08/10 16:13:54, stevenjb wrote: > for (auto code : language_codes) Done (using const auto&) https://codereview.chromium.org/1283603002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:98: ++it) { On 2015/08/10 16:13:54, stevenjb wrote: > for (auto it : language_map) Done (using const auto&, and renaming because it's not an iterator) https://codereview.chromium.org/1283603002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:213: TranslateService::GetTargetLanguage(prefs); On 2015/08/10 16:13:54, stevenjb wrote: > nit: no need for local var Done. Hope this isn't too much. https://codereview.chromium.org/1283603002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/language_settings_private/language_settings_private_delegate.cc (right): https://codereview.chromium.org/1283603002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_delegate.cc:86: listening_spellcheck_ = false; On 2015/08/10 16:13:54, stevenjb wrote: > nit: This would be more clear inside the if() body Done. https://codereview.chromium.org/1283603002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/language_settings_private/language_settings_private_delegate.h (right): https://codereview.chromium.org/1283603002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_delegate.h:39: scoped_ptr<api::language_settings_private::SpellcheckDictionaryStatus>> On 2015/08/10 16:13:54, stevenjb wrote: > ScopedVector? Done.
michaelpg@chromium.org changed reviewers: + estade@chromium.org - shuchen@chromium.org
On second thought: -shuchen@ (you can review the IME changes later, though!) +estade@: could you take a look? These will be used for new settings, but let me know if there are pitfalls with the existing code that you've noticed which can be prevented here.
estade@chromium.org changed reviewers: + hajimehoshi@chromium.org, nona@chromium.org
I don't have any particular knowledge of the language settings page. +nona, hajimehoshi
I'm afraid I know next to nothing about extensions.
On 2015/08/11 04:21:24, hajimehoshi wrote: > I'm afraid I know next to nothing about extensions. michaelpg is looking for someone familiar with the language settings page, not extensions. michaelpg: Perhaps some guidance about what to look at and provide feedback on would help.
On 2015/08/11 17:54:00, Evan Stade wrote: > On 2015/08/11 04:21:24, hajimehoshi wrote: > > I'm afraid I know next to nothing about extensions. > > michaelpg is looking for someone familiar with the language settings page, not > extensions. michaelpg: Perhaps some guidance about what to look at and provide > feedback on would help. stevenjb@ is a capable reviewer for extension APIs, but we'd like someone familiar with the language options C++ handler as well. This is basically a lightweight API version of LanguageOptionsHandler*. The API intentionally defers some logic to the JavaScript, but if anything in this implementation jumps out as silly/wrong, that would be good to know. Since the language options handler was written >4 years ago, anyone familiar with Languages in general would also be helpful. The meat of the changes is in language_settings_private_delegate.cc and the first big function in language_settings_private_api.cc. TL;DR: If anyone wants to take a look, I'd appreciate it, but it isn't strictly necessary. Thanks! Michael
I'm sorry I'm almost 3 years from last my commitment to the Chrome, so I may not be a good reviewer. Also I'm not familiar with spell checker stuff. I briefly reviewed but please get final approval from appropriate owners. https://codereview.chromium.org/1283603002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc (right): https://codereview.chromium.org/1283603002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:88: translate::TranslateDownloadManager::GetSupportedLanguages( Is this function called different from UI thread? If yes, please double check TranslateDownloadManager::GetSupportedLanguages is thread-safe or not. Looks like TranslateDownloadManager::all_supported_languages_ can be accessed from UI thread. (I'm sorry if this is not the case). FYI: as far as I know l10n_util is thread-safe. Also spellcheck_common::SpellCheckLanguages is safe to call from non UI thread.
lgtm with a couple of nits/suggestions. https://codereview.chromium.org/1283603002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc (right): https://codereview.chromium.org/1283603002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:64: LanguageMap language_map(c); On 2015/08/10 23:31:50, michaelpg wrote: > On 2015/08/10 16:13:54, stevenjb wrote: > > nit: elim |c| > > I don't write enough C++ these days to be confident on this, so please check my > logic: > > 1. it has to be 2 lines anyway; collator.get() is an argument to a regular > explicit constructor, not a copy constructor or anything, so the class name is > still necessary > > 2. the StringComparator being passed as a const reference, which std::map then > makes and stores a copy of. StringComparator has an implicit, trivial copy > constructor, so this is safe -- the temporary StringComparator created here can > be destroyed immediately Right. It's just a nit, but the reasoning is: declaring a local variable suggests that it might be used elsewhere in the function. Using an implicit temporary makes it clear that it is just needed to construct language_map. https://codereview.chromium.org/1283603002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:213: TranslateService::GetTargetLanguage(prefs); On 2015/08/10 23:31:50, michaelpg wrote: > On 2015/08/10 16:13:54, stevenjb wrote: > > nit: no need for local var > > Done. Hope this isn't too much. Occasionally, a well named local used only once helps clarify the code, e.g. first_name = args[0]. Other times it is important to test or DCHECK assumptions. Here, I think that eliminating the locals simplifies the code and makes it more readable, so cheers. https://codereview.chromium.org/1283603002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/language_settings_private/language_settings_private_delegate.cc (right): https://codereview.chromium.org/1283603002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_delegate.cc:158: } This code is duplicated in Shutdown, we should move it to a helper. https://codereview.chromium.org/1283603002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_delegate.cc:245: ret; s/ret/result/ (or something more clear... short for 'return'?) https://codereview.chromium.org/1283603002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/language_settings_private/language_settings_private_delegate.h (right): https://codereview.chromium.org/1283603002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_delegate.h:72: const std::vector<base::WeakPtr<SpellcheckHunspellDictionary>>& We should typedef this since it is also used in the C++ code and below.
https://codereview.chromium.org/1283603002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc (right): https://codereview.chromium.org/1283603002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:88: translate::TranslateDownloadManager::GetSupportedLanguages( On 2015/08/11 18:48:35, Seigo Nonaka wrote: > Is this function called different from UI thread? > If yes, please double check TranslateDownloadManager::GetSupportedLanguages is > thread-safe or not. > Looks like TranslateDownloadManager::all_supported_languages_ can be accessed > from UI thread. (I'm sorry if this is not the case). I don't think this function is thread-safe. https://codereview.chromium.org/1283603002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/language_settings_private/language_settings_private_delegate.cc (right): https://codereview.chromium.org/1283603002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_delegate.cc:176: LanguageSettingsPrivateDelegate::GetHunspellDictionaryStatuses() { nit: indent https://codereview.chromium.org/1283603002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_delegate.cc:197: LanguageSettingsPrivateDelegate::GetHunspellDictionaries() { nit: indent
https://codereview.chromium.org/1283603002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc (right): https://codereview.chromium.org/1283603002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:88: translate::TranslateDownloadManager::GetSupportedLanguages( On 2015/08/12 05:34:48, hajimehoshi wrote: > On 2015/08/11 18:48:35, Seigo Nonaka wrote: > > Is this function called different from UI thread? > > If yes, please double check TranslateDownloadManager::GetSupportedLanguages is > > thread-safe or not. > > Looks like TranslateDownloadManager::all_supported_languages_ can be accessed > > from UI thread. (I'm sorry if this is not the case). > > I don't think this function is thread-safe. Could you elaborate on why not? language_options_handler_common.cc currently calls this on the UI thread, just like here. GetSupportedLanguages calls TranslateLanguageList::GetSupportedLanguages. The only thing that jumps out at me is TranslateLanguageList::SetResourceRequestsAllowed, called by TranslateService::OnResourceRequestsAllowed using a ResourceRequestAllowedNotifier which I have no idea what thread it might run on... https://codereview.chromium.org/1283603002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:88: translate::TranslateDownloadManager::GetSupportedLanguages( On 2015/08/11 18:48:35, Seigo Nonaka wrote: > Is this function called different from UI thread? > If yes, please double check TranslateDownloadManager::GetSupportedLanguages is > thread-safe or not. This function is called on the UI thread (it's part of a UIThreadExtensionFunction). > Looks like TranslateDownloadManager::all_supported_languages_ can be accessed > from UI thread. (I'm sorry if this is not the case). > > FYI: as far as I know l10n_util is thread-safe. Also > spellcheck_common::SpellCheckLanguages is safe to call from non UI thread. https://codereview.chromium.org/1283603002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/language_settings_private/language_settings_private_delegate.cc (right): https://codereview.chromium.org/1283603002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_delegate.cc:158: } On 2015/08/11 19:30:19, stevenjb wrote: > This code is duplicated in Shutdown, we should move it to a helper. Done. https://codereview.chromium.org/1283603002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_delegate.cc:176: LanguageSettingsPrivateDelegate::GetHunspellDictionaryStatuses() { On 2015/08/12 05:34:49, hajimehoshi wrote: > nit: indent Done. https://codereview.chromium.org/1283603002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_delegate.cc:197: LanguageSettingsPrivateDelegate::GetHunspellDictionaries() { On 2015/08/12 05:34:49, hajimehoshi wrote: > nit: indent Done. https://codereview.chromium.org/1283603002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_delegate.cc:245: ret; On 2015/08/11 19:30:19, stevenjb wrote: > s/ret/result/ (or something more clear... short for 'return'?) Done. https://codereview.chromium.org/1283603002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/language_settings_private/language_settings_private_delegate.h (right): https://codereview.chromium.org/1283603002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_delegate.h:72: const std::vector<base::WeakPtr<SpellcheckHunspellDictionary>>& On 2015/08/11 19:30:19, stevenjb wrote: > We should typedef this since it is also used in the C++ code and below. Done. https://codereview.chromium.org/1283603002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/api/language_settings_private/language_settings_private_delegate.cc (right): https://codereview.chromium.org/1283603002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_delegate.cc:93: void LanguageSettingsPrivateDelegate::Shutdown() { Well I moved one function to match the .h order, and it kind of destroyed the diff. The only changes to this whole chunk are the two indentation changes and substituting RemoveDictionaryObservers().
lgtm https://codereview.chromium.org/1283603002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc (right): https://codereview.chromium.org/1283603002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:88: translate::TranslateDownloadManager::GetSupportedLanguages( On 2015/08/14 00:10:49, michaelpg wrote: > On 2015/08/11 18:48:35, Seigo Nonaka wrote: > > Is this function called different from UI thread? > > If yes, please double check TranslateDownloadManager::GetSupportedLanguages is > > thread-safe or not. > > This function is called on the UI thread (it's part of a > UIThreadExtensionFunction). > > > Looks like TranslateDownloadManager::all_supported_languages_ can be accessed > > from UI thread. (I'm sorry if this is not the case). > > > > FYI: as far as I know l10n_util is thread-safe. Also > > spellcheck_common::SpellCheckLanguages is safe to call from non UI thread. > Okay, I'm sorry I was not aware of UIThreadExtensionFunction. If this runs on UI thread, no problem. Thank you for checking.
On 2015/08/14 00:18:47, Seigo Nonaka wrote: > lgtm > > https://codereview.chromium.org/1283603002/diff/60001/chrome/browser/extensio... > File > chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc > (right): > > https://codereview.chromium.org/1283603002/diff/60001/chrome/browser/extensio... > chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:88: > translate::TranslateDownloadManager::GetSupportedLanguages( > On 2015/08/14 00:10:49, michaelpg wrote: > > On 2015/08/11 18:48:35, Seigo Nonaka wrote: > > > Is this function called different from UI thread? > > > If yes, please double check TranslateDownloadManager::GetSupportedLanguages > is > > > thread-safe or not. > > > > This function is called on the UI thread (it's part of a > > UIThreadExtensionFunction). > > > > > Looks like TranslateDownloadManager::all_supported_languages_ can be > accessed > > > from UI thread. (I'm sorry if this is not the case). > > > > > > FYI: as far as I know l10n_util is thread-safe. Also > > > spellcheck_common::SpellCheckLanguages is safe to call from non UI thread. > > > > Okay, I'm sorry I was not aware of UIThreadExtensionFunction. > If this runs on UI thread, no problem. > Thank you for checking. lgtm
Patchset #4 (id:100001) has been deleted
The CQ bit was checked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/1283603002/#ps80001 (title: "feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1283603002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1283603002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Apparently std::unordered_set is C++11 and doesn't compile on Mac. I've switched to base::hash_set<>. Apparently this is defined as std::unordered_map on MSVC but a regular GNU hash_map elsewhere.
On 2015/08/15 01:13:02, michaelpg wrote: > Apparently std::unordered_set is C++11 and doesn't compile on Mac. > > I've switched to base::hash_set<>. Apparently this is defined as > std::unordered_map on MSVC but a regular GNU hash_map elsewhere. Er, meant to say unordered_set, hash_set, etc. No maps here.
The CQ bit was checked by michaelpg@chromium.org
The CQ bit was unchecked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hajimehoshi@chromium.org, stevenjb@chromium.org, nona@chromium.org Link to the patchset: https://codereview.chromium.org/1283603002/#ps120001 (title: "std::unordered_set --> base::hash_set")
The CQ bit was checked by michaelpg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1283603002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1283603002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
michaelpg@chromium.org changed reviewers: + jshin@chromium.org
jshin: Please take a look and let me know if I'm off-base. Without changing operator() to const, this fails to compile only on linux_chromium_asan_rel_ng: http://pastebin.com/W5JTc0fG http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... Am I using the collator incorrectly, or is this OK?
On 2015/08/18 18:21:21, michaelpg wrote: > jshin: Please take a look and let me know if I'm off-base. > > Without changing operator() to const, this fails to compile only on > linux_chromium_asan_rel_ng: http://pastebin.com/W5JTc0fG > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... > > Am I using the collator incorrectly, or is this OK? Looks like map needs the compare function to be const, but that depends on the library implementation (see [1], although the explanation there doesn't make sense to me). I think you could fix this with [2] (it at least compiles fine for me). [1] http://stackoverflow.com/questions/13148513/does-stdmap-require-the-comparato... [2] http://pastebin.com/pKmbYLRj https://codereview.chromium.org/1283603002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc (right): https://codereview.chromium.org/1283603002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:59: scoped_ptr<icu::Collator> collator(icu::Collator::createInstance( isn't it expensive to create a collator? I thought we generally tried to cache them (e.g. make it a member var or whatever)
On 2015/08/18 18:57:59, Evan Stade wrote: > On 2015/08/18 18:21:21, michaelpg wrote: > > jshin: Please take a look and let me know if I'm off-base. > > > > Without changing operator() to const, this fails to compile only on > > linux_chromium_asan_rel_ng: http://pastebin.com/W5JTc0fG > > > > > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_... > > > > Am I using the collator incorrectly, or is this OK? > > Looks like map needs the compare function to be const, but that depends on the > library implementation (see [1], although the explanation there doesn't make > sense to me). Hmm, I guess the const reference to the map is coming from the "for const auto& entry : language_map" using a const_iterator. Makes sense. I think you could fix this with [2] (it at least compiles fine for > me). [2] is the change I made in this CL, although I also had to make the operator() const in the specialized template for base::string16 as that's the type I'm using. > > [1] > http://stackoverflow.com/questions/13148513/does-stdmap-require-the-comparato... > [2] http://pastebin.com/pKmbYLRj > > https://codereview.chromium.org/1283603002/diff/140001/chrome/browser/extensi... > File > chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc > (right): > > https://codereview.chromium.org/1283603002/diff/140001/chrome/browser/extensi... > chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:59: > scoped_ptr<icu::Collator> collator(icu::Collator::createInstance( > isn't it expensive to create a collator? I thought we generally tried to cache > them (e.g. make it a member var or whatever) Is it? I could cache it in the delegate but I'm not sure it's worth it. I could achieve the same results by creating an extra array of display names, sorting it with l10n_util::SortStrings16, then using the order of that array to properly order the return value. Is that preferable?
estade@chromium.org changed reviewers: + rouslan@chromium.org
https://codereview.chromium.org/1283603002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc (right): https://codereview.chromium.org/1283603002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:59: scoped_ptr<icu::Collator> collator(icu::Collator::createInstance( On 2015/08/18 18:57:59, Evan Stade wrote: > isn't it expensive to create a collator? I thought we generally tried to cache > them (e.g. make it a member var or whatever) it's easier to respond to individual topics when you use comments > michaelpg wrote: Is it? Well I dunno, but it seems like +rouslan mentions that to me at least once a month
https://codereview.chromium.org/1283603002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc (right): https://codereview.chromium.org/1283603002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:59: scoped_ptr<icu::Collator> collator(icu::Collator::createInstance( On 2015/08/18 19:21:34, Evan Stade wrote: > On 2015/08/18 18:57:59, Evan Stade wrote: > > isn't it expensive to create a collator? I thought we generally tried to cache > > them (e.g. make it a member var or whatever) > > it's easier to respond to individual topics when you use comments > > > michaelpg wrote: Is it? > > Well I dunno, but it seems like +rouslan mentions that to me at least once a > month :-) Don't create 100 collators in a loop. If this function is called infrequently, then you should be OK.
https://codereview.chromium.org/1283603002/diff/140001/chrome/browser/extensi... File chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc (right): https://codereview.chromium.org/1283603002/diff/140001/chrome/browser/extensi... chrome/browser/extensions/api/language_settings_private/language_settings_private_api.cc:59: scoped_ptr<icu::Collator> collator(icu::Collator::createInstance( On 2015/08/18 19:33:10, Rouslan wrote: > On 2015/08/18 19:21:34, Evan Stade wrote: > > On 2015/08/18 18:57:59, Evan Stade wrote: > > > isn't it expensive to create a collator? I thought we generally tried to > cache > > > them (e.g. make it a member var or whatever) > > > > it's easier to respond to individual topics when you use comments oops, for some reason I didn't think they were comments -- sorry! > > > > > michaelpg wrote: Is it? > > > > Well I dunno, but it seems like +rouslan mentions that to me at least once a > > month > > :-) Don't create 100 collators in a loop. If this function is called > infrequently, then you should be OK. Cool. It's called once when the new chrome://md-settings page loads, and as an extension function it's called asynchronously from the WebUI (although it runs on the UI thread).
LGTM on adding 'const' to operator(). It shouldn't hurt although it's not very clear to me why. As for the collator creation, it'd be ok if it's infrequent (as commented already) and the ICU data used for collator is the same (which is the case in this case).
The CQ bit was checked by michaelpg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hajimehoshi@chromium.org, stevenjb@chromium.org, nona@chromium.org Link to the patchset: https://codereview.chromium.org/1283603002/#ps140001 (title: "Fix const error in l10n_util_collator")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1283603002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1283603002/140001
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c8976b127fba81baf84029d0c9bc6ccda2e1c298 Cr-Commit-Position: refs/heads/master@{#344108} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
