Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
312023002:
Sync starting language and input method preferences (Closed)
|
Created:
6 years, 6 months ago by michaelpg Modified:
6 years ago Reviewers:
hajimehoshi, Alexander Alekseev, Dmitry Polukhin, Yuki, dzhioev (left Google), Shu Chen, spang CC:
chromium-reviews, dbeam+watch-options_chromium.org, oshima+watch_chromium.org, plundblad+watch_chromium.org, yukishiino+watch_chromium.org, aboxhall+watch_chromium.org, arv+watch_chromium.org, yuzo+watch_chromium.org, nona+watch_chromium.org, yusukes+watch_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, nkostylev+watch_chromium.org, Seigo Nonaka, Nikita (slow) Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSync starting language and input method preferences
Users who use additional Chromebooks or who recreate their accounts have
to manually set up their preferred languages, input methods and IMEs on
each device, because only the locale (display language) syncs.
We don't forcibly keep these settings in sync because different machines
may use different built-in or peripheral keyboards. But we can make the
set-up process smoother if we know what settings the user chose most
recently.
This CL creates syncable of the language and input
methods preferences, so the server always has the latest changes.
When, and only when, a user logs in and syncs for the first time on a
device, we take the local variants the user has already chosen for this
machine, and we add to them the global variants that come down from
sync. This only happens at most once. It should only be additive, not
remove any chosen settings.
Some caveats:
* If the user makes a change to one language/input setting, sync all
three interdependent settings so the sync server's settings are
always internally consistent.
BUG=298345
R=nkostylev@chromium.org, yukishiino@chromium.org, hajimehoshi@chromium.org
CC=nona@chromium.org
Committed: https://crrev.com/cceac6fae685fd86d6f347d6712886549e372231
Cr-Commit-Position: refs/heads/master@{#306164}
Committed: https://crrev.com/975f2189e0e36e367a2fc0c7147373e34f3ffc46
Cr-Commit-Position: refs/heads/master@{#306976}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Use original local preferences. #Patch Set 3 : Changes in progress, probably broken... #
Total comments: 2
Patch Set 4 : Validation and updates for tests. #Patch Set 5 : #Patch Set 6 : #
Total comments: 7
Patch Set 7 : #
Total comments: 15
Patch Set 8 : rebase and dzhioev's comments #
Total comments: 4
Patch Set 9 : Refactored for xkb refactor #
Total comments: 19
Patch Set 10 : Address comments, move InputMethodSyncer to new file #Patch Set 11 : rebase #Patch Set 12 : fix test, rebase #Patch Set 13 : fix more tests? #Patch Set 14 : rebase #
Total comments: 2
Patch Set 15 : fix null pointer dereference #Patch Set 16 : rebase #Messages
Total messages: 72 (13 generated)
Please take a look and let me know if you want to recommend other reviewers -- I wasn't sure who would be most knowledgeable here. Let me know if you have questions. The previous CL (which was put on hold while multiprofile issues were resolved) was confusing, hopefully this is simpler and well-explained. https://codereview.chromium.org/312023002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/312023002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/preferences.cc:54: // for small lists. Why not use ListValues instead of StringValues for these preferences? * The existing prefs already use strings * This function will only be called at most once per device * Given the nature of the preferences, time complexity is not a concern * There is no set-union for vectors which retains the pre-existing, non-sorted order (AFAIK) so this work is necessary anyway https://codereview.chromium.org/312023002/diff/20001/chrome/browser/resources... File chrome/browser/resources/options/language_list.js (right): https://codereview.chromium.org/312023002/diff/20001/chrome/browser/resources... chrome/browser/resources/options/language_list.js:104: preferredLanguagesPref: 'settings.language.preferred_languages_local', hajimehoshi@ asked: "Currently, 'settings.language.preferred_languages' is used as the language list at chrome://settings/languages on Chrome OS, while 'intl.accept_languages' is used on the other platforms. (On Chrome OS, when the language list is updated, both preferences are updated. Please see TranslatePrefs::UpdateLanguageList.) This change means the language list won't be synced on Chrome OS except for initializing. Is that OK?" I'm not sure I understand the question. Currently on Chrome OS the language list is not synced, and this doesn't change that. So yes, the language list will get out of date compared to Chrome desktop. But kPrefTranslateBlockedLanguages should still sync either way.
+alemate@ +shuchen@
https://codereview.chromium.org/312023002/diff/20001/chrome/browser/resources... File chrome/browser/resources/options/language_list.js (right): https://codereview.chromium.org/312023002/diff/20001/chrome/browser/resources... chrome/browser/resources/options/language_list.js:104: preferredLanguagesPref: 'settings.language.preferred_languages_local', On 2014/06/04 01:12:02, Michael Giuffrida wrote: > hajimehoshi@ asked: > > "Currently, 'settings.language.preferred_languages' is used as the language list > at chrome://settings/languages on Chrome OS, while 'intl.accept_languages' is > used on the other platforms. (On Chrome OS, when the language list is updated, > both preferences are updated. Please see TranslatePrefs::UpdateLanguageList.) > This change means the language list won't be synced on Chrome OS except for > initializing. Is that OK?" > > I'm not sure I understand the question. Currently on Chrome OS the language list > is not synced, and this doesn't change that. So yes, the language list will get > out of date compared to Chrome desktop. But kPrefTranslateBlockedLanguages > should still sync either way. AFAIK, currently, changing the language list at non-Chrome-OS doesn't affect Chrome OS, but changing the language list at Chrome OS affects non-Chrome-OS. Does this CL changes this behavior?
CC +kenjibaheux
On 2014/06/04 06:05:36, hajimehoshi wrote: > CC +kenjibaheux Note: the list of languages and some extra settings in chrome://settings/languages are used to drive Chrome Translate's behavior. No issue if these are represented by different structures that are already properly sync-ed and if chrome://settings/languages accurately reflect these. It's convenient to have Chrome Translate behave the same with identical user-visible preferences on any sync-ed device.
On 2014/06/04 06:23:28, kenjibaheux wrote: > On 2014/06/04 06:05:36, hajimehoshi wrote: > > CC +kenjibaheux > > Note: the list of languages and some extra settings in > chrome://settings/languages are used to drive Chrome Translate's behavior. No > issue if these are represented by different structures that are already properly > sync-ed and if chrome://settings/languages accurately reflect these. > > It's convenient to have Chrome Translate behave the same with identical > user-visible preferences on any sync-ed device. OK, thanks for the info. I'll take a look tomorrow [today], but I wouldn't expect this change to have side effects. Whatever used to sync before still syncs now, and apart from adding languages & inputs after OOBE nothing additional is syncing.
https://codereview.chromium.org/312023002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/312023002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/preferences.cc:260: prefs::kLanguagePreferredLanguagesLocal, I think it is dangerous to change meaning of existing preference. Would you mind adding prefs::kLanguagePreferredLanguagesSyncable ? Even if you found and renamed all existing usage of kLanguagePreferredLanguages, there might be pending CLs using kLanguagePreferredLanguages in current way. https://codereview.chromium.org/312023002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/preferences.cc:268: prefs::kLanguagePreloadEnginesLocal, The same for engines. https://codereview.chromium.org/312023002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/preferences.cc:277: "", The same for EnabledExtensionsImes https://codereview.chromium.org/312023002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/preferences.cc:458: enabled_extension_imes_.GetValue()); This is dangerous, as you do not check that remote values are supported locally. Remote values can be either from older version of chrome, or from newer version. Also you cannot guarantee that list of supported extensions is the same on all platforms.
https://codereview.chromium.org/312023002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/312023002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/preferences.cc:456: preload_engines_local_.SetValue(preload_engines_.GetValue()); The same as my comment below: the list of supported languages and extensions might be different across devices.
One concern is you may need to make sure the input method IDs in prefs are in "engine id" format instead of "_comp_ime_...xkb:..", So that it can be shared cross devices. Otherwise, there will be problem to share input methods between a ChromeOS device and a ChromiumOS device. For 3rd party IMEs, we may need to make sure the 3rd party IME extensions are sync'ed correctly before we enable the IMEs it provides.
On 2014/06/04 14:35:11, Shu Chen wrote: > One concern is you may need to make sure the input method IDs in prefs are in > "engine id" format instead of "_comp_ime_...xkb:..", So that it can be shared > cross devices. Otherwise, there will be problem to share input methods between a > ChromeOS device and a ChromiumOS device. Will do. > > For 3rd party IMEs, we may need to make sure the 3rd party IME extensions are > sync'ed correctly before we enable the IMEs it provides. Looking at input_method_manager_impl.cc, it seems that including an unknown extension IME ID won't have any effect until that extension IME gets installed & added to the input method manager, at which point it would become enabled.
On 2014/06/04 14:20:56, alemate wrote: > https://codereview.chromium.org/312023002/diff/20001/chrome/browser/chromeos/... > File chrome/browser/chromeos/preferences.cc (right): > > https://codereview.chromium.org/312023002/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/preferences.cc:260: > prefs::kLanguagePreferredLanguagesLocal, > I think it is dangerous to change meaning of existing preference. > Would you mind adding prefs::kLanguagePreferredLanguagesSyncable ? > OK. > Even if you found and renamed all existing usage of kLanguagePreferredLanguages, > there might be pending CLs using kLanguagePreferredLanguages in current way. > > https://codereview.chromium.org/312023002/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/preferences.cc:268: prefs::kLanguagePreloadEnginesLocal, > The same for engines. > > https://codereview.chromium.org/312023002/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/preferences.cc:277: "", > The same for EnabledExtensionsImes > > https://codereview.chromium.org/312023002/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/preferences.cc:458: enabled_extension_imes_.GetValue()); > This is dangerous, as you do not check that remote values are supported locally. > Remote values can be either from older version of chrome, or from newer version. > Also you cannot guarantee that list of supported extensions is the same on all > platforms. Is there anything dangerous about including a remote value that is unknown to this instance? InputMethodManagerImpl::ReplaceEnabledInputMethods filters out unknown or obsolete IDs already.
> https://codereview.chromium.org/312023002/diff/20001/chrome/browser/chromeos/... > > chrome/browser/chromeos/preferences.cc:458: > enabled_extension_imes_.GetValue()); > > This is dangerous, as you do not check that remote values are supported > locally. > > Remote values can be either from older version of chrome, or from newer > version. > > Also you cannot guarantee that list of supported extensions is the same on all > > platforms. > > Is there anything dangerous about including a remote value that is unknown to > this instance? InputMethodManagerImpl::ReplaceEnabledInputMethods filters out > unknown or obsolete IDs already. It might stay there for a long time, until it gets (suddenly) supported and user will get new behavior. You'll never find out what has happened.
https://codereview.chromium.org/312023002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/312023002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/preferences.cc:789: extension_ime_util::MaybeGetLegacyXkbId); extension_ime_util::MaybeGetLegacyXkbId to get engine id specifically for XKB input methods. You need to get engine id from input method id, so please add an util method GetEngineIDByInputMethodID in extension_ime_util for that (and use GetEngineIDByInputMethodID in MaybeGetLegacyXkbId). Thanks
On 2014/06/11 03:54:56, Shu Chen wrote: > https://codereview.chromium.org/312023002/diff/60001/chrome/browser/chromeos/... > File chrome/browser/chromeos/preferences.cc (right): > > https://codereview.chromium.org/312023002/diff/60001/chrome/browser/chromeos/... > chrome/browser/chromeos/preferences.cc:789: > extension_ime_util::MaybeGetLegacyXkbId); > extension_ime_util::MaybeGetLegacyXkbId to get engine id specifically for XKB > input methods. > You need to get engine id from input method id, so please add an util method > GetEngineIDByInputMethodID in extension_ime_util for that (and use > GetEngineIDByInputMethodID in MaybeGetLegacyXkbId). Thanks Let me make sure I understand correctly. GetEngineIDByInputMethodID would either return the xkb ID (for XKB input methods) or it would return "component extension ID + engine ID" for component extensions. So the logic would be: If input_method_id contains "xkb:", return substring starting at "xkb:" Else if input_method_id contains kComponentExtensionIMEPrefix, return substring starting at kComponentExtensionIMEPrefixLength Else return input_method_id And then MaybeGetLegacyXkbId would just redirect to GetEngineIDByInputMethodID. Is that right?
On 2014/06/13 20:21:15, Michael Giuffrida wrote: > On 2014/06/11 03:54:56, Shu Chen wrote: > > > https://codereview.chromium.org/312023002/diff/60001/chrome/browser/chromeos/... > > File chrome/browser/chromeos/preferences.cc (right): > > > > > https://codereview.chromium.org/312023002/diff/60001/chrome/browser/chromeos/... > > chrome/browser/chromeos/preferences.cc:789: > > extension_ime_util::MaybeGetLegacyXkbId); > > extension_ime_util::MaybeGetLegacyXkbId to get engine id specifically for XKB > > input methods. > > You need to get engine id from input method id, so please add an util method > > GetEngineIDByInputMethodID in extension_ime_util for that (and use > > GetEngineIDByInputMethodID in MaybeGetLegacyXkbId). Thanks > > > Let me make sure I understand correctly. > > GetEngineIDByInputMethodID would either return the xkb ID (for XKB input > methods) or it would return "component extension ID + engine ID" for component > extensions. So the logic would be: > > If input_method_id contains "xkb:", return substring starting at "xkb:" > Else if input_method_id contains kComponentExtensionIMEPrefix, return substring > starting at kComponentExtensionIMEPrefixLength > Else return input_method_id > > And then MaybeGetLegacyXkbId would just redirect to GetEngineIDByInputMethodID. > > Is that right? Correct, except MaybeGetLegacyXkbId redirects to GetEngineIDByInputMethodID and check whether the engine ID starts with "xkb:", if yes, returns engine ID, else, returns the input method ID (that's why the method is called Maybe...).
OK, I've made some updates to the syncing logic to check the languages, preload engines & extension IMEs so we don't add unsupported values to the device-local preferences. I've also updated the unit test to use real values. https://codereview.chromium.org/312023002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/312023002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/preferences.cc:789: extension_ime_util::MaybeGetLegacyXkbId); On 2014/06/11 03:54:55, Shu Chen wrote: > extension_ime_util::MaybeGetLegacyXkbId to get engine id specifically for XKB > input methods. > You need to get engine id from input method id, so please add an util method > GetEngineIDByInputMethodID in extension_ime_util for that (and use > GetEngineIDByInputMethodID in MaybeGetLegacyXkbId). Thanks Done.
https://codereview.chromium.org/312023002/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/312023002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:139: if (component_extension_manager->IsInitialized()) { This seems to be dangerous (what if Component Extension has not been initialized yet?). The problem might arise if browser process was restarted on user log in, so all initialization is (currently) started at the same time asynchronously. shuchen@ : After crbug.com/378348 - can we be sure that IME is initialized at this point?
https://codereview.chromium.org/312023002/diff/140001/chromeos/ime/extension_... File chromeos/ime/extension_ime_util.cc (right): https://codereview.chromium.org/312023002/diff/140001/chromeos/ime/extension_... chromeos/ime/extension_ime_util.cc:116: just do: return input_method_id.substr(kComponentExtensionIMEPrefixLength + kExtensionIdLength); https://codereview.chromium.org/312023002/diff/140001/chromeos/ime/extension_... File chromeos/ime/extension_ime_util.h (right): https://codereview.chromium.org/312023002/diff/140001/chromeos/ime/extension_... chromeos/ime/extension_ime_util.h:84: // Returns legacy engine id from the extension-based InputMethodID. This s/legacy// and mention only support component IME extensions.
https://codereview.chromium.org/312023002/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/312023002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:139: if (component_extension_manager->IsInitialized()) { On 2014/06/30 21:22:23, alemate wrote: > This seems to be dangerous (what if Component Extension has not > been initialized yet?). I wouldn't say "dangerous", the scenario is that during your first sync, your synced component extension IMEs don't get added locally. You would still keep any you've already added locally. And if the alternative is to wait around and check at a later time, we run the risk of adding IMEs after-the-fact. > The problem might arise if browser process was restarted on user log > in, so all initialization is (currently) started at the same time > asynchronously. How often does this happen? > > shuchen@ : After crbug.com/378348 - can we be sure that IME is initialized at > this point? https://codereview.chromium.org/312023002/diff/140001/chromeos/ime/extension_... File chromeos/ime/extension_ime_util.cc (right): https://codereview.chromium.org/312023002/diff/140001/chromeos/ime/extension_... chromeos/ime/extension_ime_util.cc:116: On 2014/07/02 02:29:04, Shu Chen wrote: > just do: > > return input_method_id.substr(kComponentExtensionIMEPrefixLength + > kExtensionIdLength); Done. https://codereview.chromium.org/312023002/diff/140001/chromeos/ime/extension_... File chromeos/ime/extension_ime_util.h (right): https://codereview.chromium.org/312023002/diff/140001/chromeos/ime/extension_... chromeos/ime/extension_ime_util.h:84: // Returns legacy engine id from the extension-based InputMethodID. This On 2014/07/02 02:29:04, Shu Chen wrote: > s/legacy// > > and mention only support component IME extensions. Done. https://codereview.chromium.org/312023002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/312023002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:109: prefs->SetBoolean(prefs::kLanguageShouldMergeInputMethods, true); This isn't actually new to patch#7. reitveld seems confused because the file has been renamed.
lgtm for ime related changes. I'm not owner for chromeos/ime/..., so you may want to get approval from yukishiino@.
Thanks, I've added yukishiino@ for chromeos/ime.
lgtm for chromeos/ime/
lgtm https://codereview.chromium.org/312023002/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/312023002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:139: if (component_extension_manager->IsInitialized()) { On 2014/07/10 00:46:01, Michael Giuffrida wrote: > On 2014/06/30 21:22:23, alemate wrote: > > This seems to be dangerous (what if Component Extension has not > > been initialized yet?). > > I wouldn't say "dangerous", the scenario is that during your first sync, your > synced component extension IMEs don't get added locally. You would still keep > any you've already added locally. And if the alternative is to wait around and > check at a later time, we run the risk of adding IMEs after-the-fact. > > > The problem might arise if browser process was restarted on user log > > in, so all initialization is (currently) started at the same time > > asynchronously. > > How often does this happen? Ok, it's not "dangerous", but it just disables sync. Restart happens when device owner (or domain policy) has non-default flags set. So adding new user will result in chrome restart. The question is if asynchronous IME initialization will work here. But it is beyond this CL. > > shuchen@ : After crbug.com/378348 - can we be sure that IME is initialized at > > this point? >
Thanks all. Nikita, does this LGTY?
I'm delegating my review to Pavel since he was working on making Chrome OS preferences work with multi-profiles and should take a look at this CL as well.
https://codereview.chromium.org/312023002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/312023002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:109: prefs->SetBoolean(prefs::kLanguageShouldMergeInputMethods, true); On 2014/07/10 00:46:02, michaelpg wrote: > This isn't actually new to patch#7. reitveld seems confused because the file has > been renamed. Are you sure? There are no such lines on ToT. https://codereview.chromium.org/312023002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/fake_user_manager.cc (right): https://codereview.chromium.org/312023002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/fake_user_manager.cc:86: (*it)->set_is_active(true); Same true for these lines. Could you please rebase on ToT?
https://codereview.chromium.org/312023002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/312023002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:109: prefs->SetBoolean(prefs::kLanguageShouldMergeInputMethods, true); On 2014/07/23 11:02:20, dzhioev wrote: > On 2014/07/10 00:46:02, michaelpg wrote: > > This isn't actually new to patch#7. reitveld seems confused because the file > has > > been renamed. > > Are you sure? There are no such lines on ToT. Sorry, didn't notice that you were talking about patch #7. Ignore my previous comments.
https://codereview.chromium.org/312023002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/312023002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:106: language_preferred_languages.SetValue(JoinString(language_codes, ',')); Could you please replace these 3 lines with a single line: prefs->SetString(prefs::kLanguagePreferredLanguages, JoinString(language_codes, ',')) https://codereview.chromium.org/312023002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/fake_user_manager.cc (right): https://codereview.chromium.org/312023002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/fake_user_manager.cc:124: (*it)->set_is_active(true); Currently active user should be marked as inactive. https://codereview.chromium.org/312023002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/312023002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:813: preferred_languages_.SetValue( I can't understand why do you set preferred languages with unchecked value, and later make checks (CheckAndResolveLocales) and set them again. Can you explain? https://codereview.chromium.org/312023002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:821: enabled_extension_imes_.SetValue( I think the better idea is to set prefs directly through prefs_ (and not through PrefMember-s). In this case we will be notified about change through |ApplyPreferences|, which knows how to handle pref changes correctly. |SetLanguageConfigStringListAsCSV|, |SetEnabledExtensionImes|, |SetSyncableInputMethodPrefs| calls below can be dropped in this case.
https://codereview.chromium.org/312023002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/312023002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:106: language_preferred_languages.SetValue(JoinString(language_codes, ',')); On 2014/07/24 20:31:34, dzhioev wrote: > Could you please replace these 3 lines with a single line: > prefs->SetString(prefs::kLanguagePreferredLanguages, JoinString(language_codes, > ',')) Done. https://codereview.chromium.org/312023002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/fake_user_manager.cc (right): https://codereview.chromium.org/312023002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/fake_user_manager.cc:86: (*it)->set_is_active(true); On 2014/07/23 11:02:20, dzhioev wrote: > Same true for these lines. Could you please rebase on ToT? Done and done. https://codereview.chromium.org/312023002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/fake_user_manager.cc:124: (*it)->set_is_active(true); On 2014/07/24 20:31:34, dzhioev wrote: > Currently active user should be marked as inactive. Done. https://codereview.chromium.org/312023002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/312023002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:813: preferred_languages_.SetValue( On 2014/07/24 20:31:34, dzhioev wrote: > I can't understand why do you set preferred languages with unchecked value, and > later make checks (CheckAndResolveLocales) and set them again. Can you explain? The check is asynchronous. So this way, the changes are made immediately (e.g., input methods are added). Removing the unsupported languages is important so that languages don't suddenly get added in the future if Chrome adds support for them, but I don't think it should block these settings. That said, the time it takes for CheckAndResolveLocales to return will probably never be that long, so if you would rather see all of the updates and checks happen once, after the CheckAndResolveLocales callback is called, I'll do it that way. https://codereview.chromium.org/312023002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:821: enabled_extension_imes_.SetValue( On 2014/07/24 20:31:34, dzhioev wrote: > I think the better idea is to set prefs directly through prefs_ (and not through > PrefMember-s). In this case we will be notified about change through > |ApplyPreferences|, which knows how to handle pref changes correctly. > |SetLanguageConfigStringListAsCSV|, |SetEnabledExtensionImes|, > |SetSyncableInputMethodPrefs| calls below can be dropped in this case. Does this look better? When any of these prefs are changed, SetInputMethodsSyncable() is called, which sets all 3 of the syncable prefs at the same time. This is important so that we don't push one preference from this machine while the server still has two prefs from another machine with a totally different setup. I thought about adding a comment to explain that, but I think this is a simpler solution and it lets me set the prefs directly: Cache the syncable prefs, so that we don't care when they're changed, and then we can set the prefs directly and don't have to duplicate the change handlers.
Sorry for a long delay, I was sick all week. https://codereview.chromium.org/312023002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/312023002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:813: preferred_languages_.SetValue( On 2014/07/25 23:49:08, michaelpg wrote: > On 2014/07/24 20:31:34, dzhioev wrote: > > I can't understand why do you set preferred languages with unchecked value, > and > > later make checks (CheckAndResolveLocales) and set them again. Can you > explain? > > The check is asynchronous. So this way, the changes are made immediately (e.g., > input methods are added). Removing the unsupported languages is important so > that languages don't suddenly get added in the future if Chrome adds support for > them, but I don't think it should block these settings. I understand part about checking. But still I don't understand why we need to set |preferred_languages_| twice. First we set it here (with unchecked values), then in the end of this method we call CheckAndResolveLocales and after it finished we call SetPreferredLanguages which sets |preferred_languages_| again, but with values that were filtered of unsupported languages. > That said, the time it takes for CheckAndResolveLocales to return will probably > never be that long, so if you would rather see all of the updates and checks > happen once, after the CheckAndResolveLocales callback is called, I'll do it > that way. Yep, I prefer that way. BTW, don't we need to check other two preferences? https://codereview.chromium.org/312023002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:821: enabled_extension_imes_.SetValue( Yes, it's better. https://codereview.chromium.org/312023002/diff/180001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/fake_user_manager.cc (right): https://codereview.chromium.org/312023002/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/fake_user_manager.cc:160: } nit: I think '(*it)->set_is_active((*it)->email() == active_user_id_)' is a more straight way to do this.
https://codereview.chromium.org/312023002/diff/180001/chromeos/ime/extension_... File chromeos/ime/extension_ime_util.h (right): https://codereview.chromium.org/312023002/diff/180001/chromeos/ime/extension_... chromeos/ime/extension_ime_util.h:87: std::string CHROMEOS_EXPORT GetEngineIDByInputMethodID( You can remove this function now. The ToT now already have method GetComponentIDByInputMethodID() which does the same thing.
Hello Michael, are you going to finish this CL? This feature is on top of my personal desired features list, so I want it to be landed finally =)
Patchset #9 (id:200001) has been deleted
On 2014/10/14 13:37:20, dzhioev wrote: > Hello Michael, > are you going to finish this CL? > This feature is on top of my personal desired features list, so I want it to be > landed finally =) I feel your pain. :-( I've finished reconciling the patch with the recent IME/xkb changes. It works as expected on CrOS for Linux, but fails to sync on test devices. I've blocked out time this week to debug/figure out why to hopefully have the final patch up real soon.
Patchset #9 (id:220001) has been deleted
Please take (yet) another look: * Handles the recent xkb overhaul. * This was getting way too complicated. I've now encapsulated all the CL-specific logic into "InputMethodSyncer", clearing up the confusion in the Preferences class and making the process easier to follow. If we can get this in before another major change to input methods, I will be very happy :-) https://codereview.chromium.org/312023002/diff/180001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/fake_user_manager.cc (right): https://codereview.chromium.org/312023002/diff/180001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/fake_user_manager.cc:160: } On 2014/08/01 15:30:41, dzhioev wrote: > nit: I think '(*it)->set_is_active((*it)->email() == active_user_id_)' is a more > straight way to do this. Done. https://codereview.chromium.org/312023002/diff/180001/chromeos/ime/extension_... File chromeos/ime/extension_ime_util.h (right): https://codereview.chromium.org/312023002/diff/180001/chromeos/ime/extension_... chromeos/ime/extension_ime_util.h:87: std::string CHROMEOS_EXPORT GetEngineIDByInputMethodID( On 2014/08/07 14:58:51, Shu Chen wrote: > You can remove this function now. The ToT now already have method > GetComponentIDByInputMethodID() which does the same thing. Acknowledged.
Drive by comment. Heads up: both Pavel and Alexander are ooo till Nov 5th (vacation/offsite/public holidays). https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:139: class InputMethodSyncer : public PrefServiceSyncableObserver { I wonder if this class should be extracted into a separate file, this way it is possible to add unit_tests for it.
https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:60: const input_method::InputMethodDescriptors& supported_descriptors) { Input arguments should go first. https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:75: *it) != supported_input_method_ids.end()) { Maybe a bit shorter: supported_input_method_ids.find(*it) != supported_input_method_ids.end() https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:85: void CheckAndResolveLocales(std::string* languages) { DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:99: if (std::find(accept_language_codes.begin(), accept_language_codes.end(), You traverse accept_language_codes linearly twice here. Maybe sort it and use std::binary_search ? https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:139: class InputMethodSyncer : public PrefServiceSyncableObserver { On 2014/10/21 11:45:56, Nikita Kostylev wrote: > I wonder if this class should be extracted into a separate file, this way it is > possible to add unit_tests for it. +1 https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:315: content::BrowserThread::FILE, AFAIK, FILE thread is deprecated. BrowserThread::GetBlockingPool()->PostTaskAndReply(...);
Sorry for the long delay. I was thinking if merge of local + remote preferences is correct. I think, it is not (because it doesn't work if someone wants to drop most common keyboard in favor of DVORAK or something like that). But I could not to invent any better way to solve this issue. So let's try and gather feedback. On 2014/10/28 15:11:00, Alexander Alekseev wrote: > https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... > File chrome/browser/chromeos/preferences.cc (right): > > https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... > chrome/browser/chromeos/preferences.cc:60: const > input_method::InputMethodDescriptors& supported_descriptors) { > Input arguments should go first. > > https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... > chrome/browser/chromeos/preferences.cc:75: *it) != > supported_input_method_ids.end()) { > Maybe a bit shorter: > > supported_input_method_ids.find(*it) != supported_input_method_ids.end() > > https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... > chrome/browser/chromeos/preferences.cc:85: void > CheckAndResolveLocales(std::string* languages) { > DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); > > https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... > chrome/browser/chromeos/preferences.cc:99: if > (std::find(accept_language_codes.begin(), accept_language_codes.end(), > You traverse accept_language_codes linearly twice here. > Maybe sort it and use std::binary_search ? > > https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... > chrome/browser/chromeos/preferences.cc:139: class InputMethodSyncer : public > PrefServiceSyncableObserver { > On 2014/10/21 11:45:56, Nikita Kostylev wrote: > > I wonder if this class should be extracted into a separate file, this way it > is > > possible to add unit_tests for it. > > +1 > > https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... > chrome/browser/chromeos/preferences.cc:315: content::BrowserThread::FILE, > AFAIK, FILE thread is deprecated. > > BrowserThread::GetBlockingPool()->PostTaskAndReply(...);
On 2014/10/28 15:14:59, Alexander Alekseev wrote: > Sorry for the long delay. I was thinking if merge of local + remote preferences > is correct. > I think, it is not (because it doesn't work if someone wants to drop most common > keyboard in favor of DVORAK or something like that). > But I could not to invent any better way to solve this issue. So let's try and > gather feedback. I don't think we should even drop what the user chooses at OOBE. In fact, I'm reluctant to let this change remove any layouts from the device, because that's different from how it works today, and it is riskier than just adding layouts. If a user wants to use an alternate layout like Dvorak, they can select that at OOBE, and whatever behavior we use currently should persist (in my experience it still adds QWERTY for some reason, but Dvorak is the selected input method). Let's keep talking about this because right now I don't know a good way to choose when to retain the original input method. > > On 2014/10/28 15:11:00, Alexander Alekseev wrote: > > > https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... > > File chrome/browser/chromeos/preferences.cc (right): > > > > > https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... > > chrome/browser/chromeos/preferences.cc:60: const > > input_method::InputMethodDescriptors& supported_descriptors) { > > Input arguments should go first. > > > > > https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... > > chrome/browser/chromeos/preferences.cc:75: *it) != > > supported_input_method_ids.end()) { > > Maybe a bit shorter: > > > > supported_input_method_ids.find(*it) != supported_input_method_ids.end() > > > > > https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... > > chrome/browser/chromeos/preferences.cc:85: void > > CheckAndResolveLocales(std::string* languages) { > > DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); > > > > > https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... > > chrome/browser/chromeos/preferences.cc:99: if > > (std::find(accept_language_codes.begin(), accept_language_codes.end(), > > You traverse accept_language_codes linearly twice here. > > Maybe sort it and use std::binary_search ? > > > > > https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... > > chrome/browser/chromeos/preferences.cc:139: class InputMethodSyncer : public > > PrefServiceSyncableObserver { > > On 2014/10/21 11:45:56, Nikita Kostylev wrote: > > > I wonder if this class should be extracted into a separate file, this way it > > is > > > possible to add unit_tests for it. > > > > +1 > > > > > https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... > > chrome/browser/chromeos/preferences.cc:315: content::BrowserThread::FILE, > > AFAIK, FILE thread is deprecated. > > > > BrowserThread::GetBlockingPool()->PostTaskAndReply(...);
toyoshim@chromium.org changed reviewers: - toyoshim@chromium.org
Moved toyoshim@ from reviewers to CC.
LGTM with nits https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:242: prefs_, callback); nit: fix indentation. https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:244: prefs_, callback); nit: fix indentation.
https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:126: for (size_t i = 0; i < src.size(); i++) { nit: for (const std::string& token: src)
nkostylev@chromium.org changed reviewers: - nkostylev@chromium.org
Patchset #10 (id:260001) has been deleted
Moved the InputMethodSyncer class from preferences.cc to input_method/input_method_syncer.cc. I'll add unit tests for this file in the next few days, but this CL is already large enough as it is, so I'd rather submit them separately. Most of the behavior is already being tested in preferences_unittest.cc anyway. https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... File chrome/browser/chromeos/preferences.cc (right): https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:60: const input_method::InputMethodDescriptors& supported_descriptors) { On 2014/10/28 15:11:00, Alexander Alekseev wrote: > Input arguments should go first. Done. https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:75: *it) != supported_input_method_ids.end()) { On 2014/10/28 15:11:00, Alexander Alekseev wrote: > Maybe a bit shorter: > > supported_input_method_ids.find(*it) != supported_input_method_ids.end() Done. https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:85: void CheckAndResolveLocales(std::string* languages) { On 2014/10/28 15:11:00, Alexander Alekseev wrote: > DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); Done. https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:99: if (std::find(accept_language_codes.begin(), accept_language_codes.end(), On 2014/10/28 15:11:00, Alexander Alekseev wrote: > You traverse accept_language_codes linearly twice here. > Maybe sort it and use std::binary_search ? Done. https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:126: for (size_t i = 0; i < src.size(); i++) { On 2014/11/06 16:29:07, dzhioev wrote: > nit: for (const std::string& token: src) Done. https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:139: class InputMethodSyncer : public PrefServiceSyncableObserver { On 2014/10/28 15:11:00, Alexander Alekseev wrote: > On 2014/10/21 11:45:56, Nikita Kostylev wrote: > > I wonder if this class should be extracted into a separate file, this way it > is > > possible to add unit_tests for it. > > +1 Done. https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:242: prefs_, callback); On 2014/11/06 16:11:06, dzhioev wrote: > nit: fix indentation. Done. https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:244: prefs_, callback); On 2014/11/06 16:11:06, dzhioev wrote: > nit: fix indentation. Done. https://codereview.chromium.org/312023002/diff/240001/chrome/browser/chromeos... chrome/browser/chromeos/preferences.cc:315: content::BrowserThread::FILE, On 2014/10/28 15:11:00, Alexander Alekseev wrote: > AFAIK, FILE thread is deprecated. > > BrowserThread::GetBlockingPool()->PostTaskAndReply(...); Done.
lgtm
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/312023002/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
michaelpg@chromium.org changed reviewers: + dpolukhin@chromium.org
@dpolukhin: this needs a review for chrome/browser/chromeos. while nkostylev@ is OOO, could you take a look? Thanks.
lgtm
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/312023002/360001
Message was sent while issue was closed.
Committed patchset #14 (id:360001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/cceac6fae685fd86d6f347d6712886549e372231 Cr-Commit-Position: refs/heads/master@{#306164}
Message was sent while issue was closed.
spang@chromium.org changed reviewers: + spang@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/312023002/diff/360001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_syncer.cc (right): https://codereview.chromium.org/312023002/diff/360001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_syncer.cc:241: base::Passed(&languages))); It is not safe to .Pass() and .get() in different arguments to the same function
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:360001) has been created in https://codereview.chromium.org/756363003/ by spang@chromium.org. The reason for reverting is: Causes NULL pointer dereference. #0 chromeos::input_method::(anonymous namespace)::CheckAndResolveLocales (languages=0x0) at ../../chrome/browser/chromeos/input_method/input_method_syncer.cc:58 #1 0x00007f9466f9ddd2 in Run (this=<optimized out>) at ../../base/callback.h:396 #2 base::(anonymous namespace)::PostTaskAndReplyRelay::Run (this=0x647e3eb28c0) at ../../base/threading/post_task_and_reply_impl.cc:42 #3 0x00007f9466fa1257 in Run (this=0x7f9454cb1b10) at ../../base/callback.h:396 #4 base::SequencedWorkerPool::Inner::ThreadLoop (this=0x647e2117000, this_worker=this_worker@entry=0x647e34f5190) at ../../base/threading/sequenced_worker_pool.cc:760 #5 0x00007f9466fa199d in base::SequencedWorkerPool::Worker::Run (this=0x647e34f5190) at ../../base/threading/sequenced_worker_pool.cc:507 #6 0x00007f9466fa1d51 in base::SimpleThread::ThreadMain (this=0x647e34f5190) at ../../base/threading/simple_thread.cc:60 #7 0x00007f9466f9db30 in base::(anonymous namespace)::ThreadFunc (params=<optimized out>) at ../../base/threading/platform_thread_posix.cc:80 #8 0x00007f94654ae321 in start_thread (arg=0x7f9454cb2700) at pthread_create.c:309 #9 0x00007f94649527ed in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:111 .
Message was sent while issue was closed.
Would someone mind taking another look? I've made a few changes to input_method_syncer.* to fix the null pointer dereference... the pointers aren't really necessary anyway IMO. https://codereview.chromium.org/312023002/diff/360001/chrome/browser/chromeos... File chrome/browser/chromeos/input_method/input_method_syncer.cc (right): https://codereview.chromium.org/312023002/diff/360001/chrome/browser/chromeos... chrome/browser/chromeos/input_method/input_method_syncer.cc:241: base::Passed(&languages))); On 2014/12/01 19:52:04, spang wrote: > It is not safe to .Pass() and .get() in different arguments to the same function Sorry about this. It never came up in my testing but I see the issue now.
Message was sent while issue was closed.
LGTM Passing string by value is better IMHO.
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/312023002/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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/312023002/400001
Message was sent while issue was closed.
Committed patchset #16 (id:400001)
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/975f2189e0e36e367a2fc0c7147373e34f3ffc46 Cr-Commit-Position: refs/heads/master@{#306976} |
