Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1102693002:
languagesPrivate IDL draft (Closed)
|
Created:
5 years, 8 months ago by michaelpg Modified:
5 years, 5 months ago CC:
chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionIDL draft for languagesPrivate API.
This will be used to get and set language and input method settings in Material Design Settings.
BUG=479043
Patch Set 1 #Patch Set 2 : kill newline #
Total comments: 26
Patch Set 3 : Feedback #
Total comments: 16
Patch Set 4 : Changes for issue 1155093006 #Messages
Total messages: 18 (6 generated)
stevenjb@chromium.org changed reviewers: + kalman@chromium.org, stevenjb@chromium.org
The language stuff all seems fine to me. As a developer looking at this API, it would seems very strange to me that the input method part, which appears to be completely independent of the language part, would be here instead of in inputMethodPrivate. See my comments in the API design doc. https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... File chrome/common/extensions/api/languages_private.idl (right): https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... chrome/common/extensions/api/languages_private.idl:25: DOMString currentLocale; Does this match 'Language.code'? If so, maybe we should name them the same, i.e. s/code/locale above? https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... chrome/common/extensions/api/languages_private.idl:42: DOMString languageCode; Same as Language.code? or currentLocale? or different? https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... chrome/common/extensions/api/languages_private.idl:62: DOMString[] languages; code? locale? https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... chrome/common/extensions/api/languages_private.idl:83: callback InputMethodEnabledDisabledCallback = void (DOMString id); This name is a bit odd, see below. https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... chrome/common/extensions/api/languages_private.idl:102: static void setInputMethodEnabled(DOMString inputMethodId, boolean enabled); Can more than one input method be enabled at once? I am curious (and a bit concerned) about how this interacts with inputMethodPrivate.setCurrentInputMethod(). https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... chrome/common/extensions/api/languages_private.idl:117: InputMethodEnabledDisabledCallback callback); Might these be better as a single event with a booelean 'enabled' state, e.g. onInputMethodEnabledStateChanged(id, isEnabled)?
https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... File chrome/common/extensions/api/languages_private.idl (right): https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... chrome/common/extensions/api/languages_private.idl:17: boolean supportsTranslate; consider making the booleans optional (and default-false; reword them if the common case is for them to be true, i.e. "disableSpellCheck" and "disableTranslate" if appropriate) so that you don't need to shuffle around redundant data. https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... chrome/common/extensions/api/languages_private.idl:25: DOMString currentLocale; On 2015/04/24 00:00:20, stevenjb wrote: > Does this match 'Language.code'? If so, maybe we should name them the same, i.e. > s/code/locale above? Is this something a web API can give you anyway? like navigator.language or whatever? https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... chrome/common/extensions/api/languages_private.idl:34: DOMString spellCheckLanguage; What does translate/spellcheck data have to do with "LanguagesInfo" struct? "supports", sure, but the actual language? https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... chrome/common/extensions/api/languages_private.idl:37: boolean autoCorrectEnabled; Ditto(s) https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... chrome/common/extensions/api/languages_private.idl:56: DOMString? extensionName; You will probably want to fetch other things, like the icon, so it would be better composed if you were to send down an ID then use another API (like chrome.management) to fetch the relevant data about it. Or, just plan to add the icon to this field I suppose. That would save some JS code, but not compose as well. Also: what language will the extension name be? https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... chrome/common/extensions/api/languages_private.idl:95: // Attempts to download the spell check dictionary after a failure. what triggers the fetch in the first place?
https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... File chrome/common/extensions/api/languages_private.idl (right): https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... chrome/common/extensions/api/languages_private.idl:42: DOMString languageCode; On 2015/04/24 00:00:20, stevenjb wrote: > Same as Language.code? or currentLocale? or different? > No, you can use any language for spell checking. So this is not the same as currentLocale. Yes, it would be the value of the ".code" for some Language. https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... chrome/common/extensions/api/languages_private.idl:95: // Attempts to download the spell check dictionary after a failure. On 2015/04/24 16:53:43, kalman wrote: > what triggers the fetch in the first place? In current Settings, if a d/l fails there's a "Retry" button the user may click. https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... chrome/common/extensions/api/languages_private.idl:102: static void setInputMethodEnabled(DOMString inputMethodId, boolean enabled); On 2015/04/24 00:00:20, stevenjb wrote: > Can more than one input method be enabled at once? > > I am curious (and a bit concerned) about how this interacts with > inputMethodPrivate.setCurrentInputMethod(). Yes, any number of input methods can be enabled. For example, I have US QWERTY and Dvorak enabled. I switch between them using Alt+Shift, which has the same effect as inputMethodPrivate.setCurrentInputMethod('qwerty ID'/'dvorak ID'). inputMethodPrivate.setCurrentInputMethod will fail unless the input method to make active is already enabled. https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... The logic for setInputMethodEnabled is already implemented as a chrome.send call in CrosLanguageOptionsHandler. But Settings doesn't directly set which input method is current. https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... chrome/common/extensions/api/languages_private.idl:117: InputMethodEnabledDisabledCallback callback); On 2015/04/24 00:00:20, stevenjb wrote: > Might these be better as a single event with a booelean 'enabled' state, e.g. > onInputMethodEnabledStateChanged(id, isEnabled)? I haven't found a single instance of a callback with multiple arguments. I assume that's possible, then?
https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... File chrome/common/extensions/api/languages_private.idl (right): https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... chrome/common/extensions/api/languages_private.idl:95: // Attempts to download the spell check dictionary after a failure. On 2015/04/24 19:08:03, michaelpg wrote: > On 2015/04/24 16:53:43, kalman wrote: > > what triggers the fetch in the first place? > > In current Settings, if a d/l fails there's a "Retry" button the user may click. Not really what I asked, but it sounds like the answer is that it's automatically triggered at some point and this page is only expected to know to Retry. However - how will it know? There is an event with the current status, but what about if the webui page isn't open at the time? https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... chrome/common/extensions/api/languages_private.idl:117: InputMethodEnabledDisabledCallback callback); On 2015/04/24 19:08:03, michaelpg wrote: > On 2015/04/24 00:00:20, stevenjb wrote: > > Might these be better as a single event with a booelean 'enabled' state, e.g. > > onInputMethodEnabledStateChanged(id, isEnabled)? > > I haven't found a single instance of a callback with multiple arguments. I > assume that's possible, then? It might be possible, though we do prefer bundling event state into a single dictionary.
https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... File chrome/common/extensions/api/languages_private.idl (right): https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... chrome/common/extensions/api/languages_private.idl:95: // Attempts to download the spell check dictionary after a failure. On 2015/04/24 20:07:25, kalman wrote: > On 2015/04/24 19:08:03, michaelpg wrote: > > On 2015/04/24 16:53:43, kalman wrote: > > > what triggers the fetch in the first place? > > > > In current Settings, if a d/l fails there's a "Retry" button the user may > click. > > Not really what I asked, but it sounds like the answer is that it's > automatically triggered at some point and this page is only expected to know to > Retry. > > However - how will it know? There is an event with the current status, but what > about if the webui page isn't open at the time? Sorry, I misread your question. The initial download happens in SpellcheckService's listener for the spellcheck.dictionary pref change -- which would be triggered when you click the "use this language for spell checking" button. You're right, this API was supposed to have a getter for the dictionary status. My API outline doc had a getSpellCheckInfo function to get that info as well as the two properties from LanguagesInfo you called out above. I'll upload v2 when I hear back from shuchen.
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #4 (id:120001) has been deleted
New draft. I'm also sending out the API proposal. Thanks! https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... File chrome/common/extensions/api/languages_private.idl (right): https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... chrome/common/extensions/api/languages_private.idl:17: boolean supportsTranslate; On 2015/04/24 16:53:43, kalman wrote: > consider making the booleans optional (and default-false; reword them if the > common case is for them to be true, i.e. "disableSpellCheck" and > "disableTranslate" if appropriate) so that you don't need to shuffle around > redundant data. Done. https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... chrome/common/extensions/api/languages_private.idl:25: DOMString currentLocale; On 2015/04/24 16:53:43, kalman wrote: > On 2015/04/24 00:00:20, stevenjb wrote: > > Does this match 'Language.code'? If so, maybe we should name them the same, > i.e. > > s/code/locale above? > > Is this something a web API can give you anyway? like navigator.language or > whatever? navigator.language corresponds to the value of intl.app_locale from startup. We want the value of intl.app_locale as it is currently set -- the user may change the display language but not restart immediately. I can move this to settingsPrivate though since it's just a basic setting. https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... chrome/common/extensions/api/languages_private.idl:25: DOMString currentLocale; On 2015/04/24 00:00:20, stevenjb wrote: > Does this match 'Language.code'? If so, maybe we should name them the same, i.e. > s/code/locale above? I'll standardize this better. Technically a locale can be described with a combination of language code, [country code], [variant code] and [maybe other stuff]. There is a one-to-many relationship between a language and its associated locales, so really just about everything in this API should be called a locale. But locale can also refer to how to format dates, Chrome UI tends to use "locale" to mean "display locale", as opposed to translate/spell check locales. https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... chrome/common/extensions/api/languages_private.idl:34: DOMString spellCheckLanguage; On 2015/04/24 16:53:42, kalman wrote: > What does translate/spellcheck data have to do with "LanguagesInfo" struct? > "supports", sure, but the actual language? LanguagesInfo was a confusing construct. Removed. https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... chrome/common/extensions/api/languages_private.idl:37: boolean autoCorrectEnabled; On 2015/04/24 16:53:43, kalman wrote: > Ditto(s) Acknowledged. https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... chrome/common/extensions/api/languages_private.idl:56: DOMString? extensionName; On 2015/04/24 16:53:43, kalman wrote: > You will probably want to fetch other things, like the icon, so it would be > better composed if you were to send down an ID then use another API (like > chrome.management) to fetch the relevant data about it. > > Or, just plan to add the icon to this field I suppose. That would save some JS > code, but not compose as well. > > Also: what language will the extension name be? You're right, we can just use the ID with chrome.management for the name, icon, options URL, etc. https://codereview.chromium.org/1102693002/diff/20001/chrome/common/extension... chrome/common/extensions/api/languages_private.idl:102: static void setInputMethodEnabled(DOMString inputMethodId, boolean enabled); On 2015/04/24 19:08:03, michaelpg wrote: > On 2015/04/24 00:00:20, stevenjb wrote: > > Can more than one input method be enabled at once? > > > > I am curious (and a bit concerned) about how this interacts with > > inputMethodPrivate.setCurrentInputMethod(). > > Yes, any number of input methods can be enabled. For example, I have US QWERTY > and Dvorak enabled. I switch between them using Alt+Shift, which has the same > effect as inputMethodPrivate.setCurrentInputMethod('qwerty ID'/'dvorak ID'). > > inputMethodPrivate.setCurrentInputMethod will fail unless the input method to > make active is already enabled. > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... > > The logic for setInputMethodEnabled is already implemented as a chrome.send call > in CrosLanguageOptionsHandler. But Settings doesn't directly set which input > method is current. So I understand the confusion with the terms "enable/disable", even though Settings doesn't control which input method is currently active. Would the terms "add/remove" be better? As in, these input methods are added and removed from the user's list of input methods.
https://codereview.chromium.org/1102693002/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/language_settings_private.idl (right): https://codereview.chromium.org/1102693002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/language_settings_private.idl:33: // The downloading status. nit: The download status of the dictionary. https://codereview.chromium.org/1102693002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/language_settings_private.idl:75: static void setUILanguage(DOMString languageCode); Question: How does the UI know what the current language is, and what the set language is? https://codereview.chromium.org/1102693002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/language_settings_private.idl:77: // Sets the accepted languages. Maybe elaborate? It's not obvious (to me) what 'accepted' means here. https://codereview.chromium.org/1102693002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/language_settings_private.idl:84: // Attempts to download the spell check dictionary after a failure. Why do we need to expose this to the UI? https://codereview.chromium.org/1102693002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/language_settings_private.idl:96: // Disables the input method for the current user. Disable (implies it remains in the list but not enabled) or Remove (implies it will no longer be in the list)?
https://codereview.chromium.org/1102693002/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/language_settings_private.idl (right): https://codereview.chromium.org/1102693002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/language_settings_private.idl:75: static void setUILanguage(DOMString languageCode); On 2015/05/11 01:25:57, stevenjb wrote: > Question: How does the UI know what the current language is, and what the set > language is? navigator.language provides the actual current UI language. The currently set language (which may be the current UI language, or if not, takes effect on signout/relaunch) is stored in Local State as intl.app_locale. On Chrome OS only, it's also stored per user in Preferences. I'll see how PrefService::FindPreference treats this, but if it finds the appropriate preference store then we can just use settingsPrivate to get/set the value. https://codereview.chromium.org/1102693002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/language_settings_private.idl:84: // Attempts to download the spell check dictionary after a failure. On 2015/05/11 01:25:57, stevenjb wrote: > Why do we need to expose this to the UI? Options currently shows a "Retry" button if the download fails. The mocks for MD-Settings don't use the dictionary status at all. I'll check w/ Tom & Cody. Are you suggesting the download retry should be handled automatically? https://codereview.chromium.org/1102693002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/language_settings_private.idl:96: // Disables the input method for the current user. On 2015/05/11 01:25:57, stevenjb wrote: > Disable (implies it remains in the list but not enabled) or Remove (implies it > will no longer be in the list)? Both? The input method will be removed from the user's list of enabled input methods. I think these method names are more precise but the comments aren't wrong. Open to suggestions for clearer documentation.
rubberstamp lgtm
https://codereview.chromium.org/1102693002/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/language_settings_private.idl (right): https://codereview.chromium.org/1102693002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/language_settings_private.idl:75: static void setUILanguage(DOMString languageCode); On 2015/05/11 08:16:01, michaelpg wrote: > On 2015/05/11 01:25:57, stevenjb wrote: > > Question: How does the UI know what the current language is, and what the set > > language is? > > navigator.language provides the actual current UI language. > > The currently set language (which may be the current UI language, or if not, > takes effect on signout/relaunch) is stored in Local State as intl.app_locale. > On Chrome OS only, it's also stored per user in Preferences. > > I'll see how PrefService::FindPreference treats this, but if it finds the > appropriate preference store then we can just use settingsPrivate to get/set the > value. As long as we document that there, I am fine with it, but it might be helpful to provide an accessor here too so that it is crystal clear what "UILanguage" refers to. https://codereview.chromium.org/1102693002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/language_settings_private.idl:84: // Attempts to download the spell check dictionary after a failure. On 2015/05/11 08:16:01, michaelpg wrote: > On 2015/05/11 01:25:57, stevenjb wrote: > > Why do we need to expose this to the UI? > > Options currently shows a "Retry" button if the download fails. > > The mocks for MD-Settings don't use the dictionary status at all. I'll check w/ > Tom & Cody. Are you suggesting the download retry should be handled > automatically? Yes. IMHO, 'Retry' represents poor/lazy UI. DownloadStatus.Failure should indicate that the dictionary is not loadable. The 'model' (i.e. Chrome) should automatically retry if/when appropriate. (If implementing that is challenging, we can make that a TODO, but we should omit any Retry UI and use something like getLanguageList() to trigger a retry so that a Settings page refresh triggers the retry). https://codereview.chromium.org/1102693002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/language_settings_private.idl:96: // Disables the input method for the current user. On 2015/05/11 08:16:02, michaelpg wrote: > On 2015/05/11 01:25:57, stevenjb wrote: > > Disable (implies it remains in the list but not enabled) or Remove (implies it > > will no longer be in the list)? > > Both? The input method will be removed from the user's list of enabled input > methods. I think these method names are more precise but the comments aren't > wrong. Open to suggestions for clearer documentation. "Removes the input method from the list of enabled methods, disabling the input method for the current user." ?
Patchset #5 (id:160001) has been deleted
I guess I never sent this out. stevenjb's feedback addressed. https://codereview.chromium.org/1102693002/diff/100001/chrome/common/extensio... File chrome/common/extensions/api/language_settings_private.idl (right): https://codereview.chromium.org/1102693002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/language_settings_private.idl:33: // The downloading status. On 2015/05/11 01:25:57, stevenjb wrote: > nit: The download status of the dictionary. > > Done. https://codereview.chromium.org/1102693002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/language_settings_private.idl:75: static void setUILanguage(DOMString languageCode); On 2015/06/01 18:08:06, stevenjb wrote: > On 2015/05/11 08:16:01, michaelpg wrote: > > On 2015/05/11 01:25:57, stevenjb wrote: > > > Question: How does the UI know what the current language is, and what the > set > > > language is? > > > > navigator.language provides the actual current UI language. > > > > The currently set language (which may be the current UI language, or if not, > > takes effect on signout/relaunch) is stored in Local State as intl.app_locale. > > On Chrome OS only, it's also stored per user in Preferences. > > > > I'll see how PrefService::FindPreference treats this, but if it finds the > > appropriate preference store then we can just use settingsPrivate to get/set > the > > value. > > As long as we document that there, I am fine with it, but it might be helpful to > provide an accessor here too so that it is crystal clear what "UILanguage" > refers to. Removed from here. https://codereview.chromium.org/1102693002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/language_settings_private.idl:77: // Sets the accepted languages. On 2015/05/11 01:25:57, stevenjb wrote: > Maybe elaborate? It's not obvious (to me) what 'accepted' means here. Done. (tl;dr: a lot of things) https://codereview.chromium.org/1102693002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/language_settings_private.idl:84: // Attempts to download the spell check dictionary after a failure. On 2015/06/01 18:08:06, stevenjb wrote: > On 2015/05/11 08:16:01, michaelpg wrote: > > On 2015/05/11 01:25:57, stevenjb wrote: > > > Why do we need to expose this to the UI? > > > > Options currently shows a "Retry" button if the download fails. > > > > The mocks for MD-Settings don't use the dictionary status at all. I'll check > w/ > > Tom & Cody. Are you suggesting the download retry should be handled > > automatically? > > Yes. IMHO, 'Retry' represents poor/lazy UI. DownloadStatus.Failure should > indicate that the dictionary is not loadable. The 'model' (i.e. Chrome) should > automatically retry if/when appropriate. (If implementing that is challenging, > we can make that a TODO, but we should omit any Retry UI and use something like > getLanguageList() to trigger a retry so that a Settings page refresh triggers > the retry). Acknowledged. https://codereview.chromium.org/1102693002/diff/100001/chrome/common/extensio... chrome/common/extensions/api/language_settings_private.idl:96: // Disables the input method for the current user. On 2015/06/01 18:08:06, stevenjb wrote: > On 2015/05/11 08:16:02, michaelpg wrote: > > On 2015/05/11 01:25:57, stevenjb wrote: > > > Disable (implies it remains in the list but not enabled) or Remove (implies > it > > > will no longer be in the list)? > > > > Both? The input method will be removed from the user's list of enabled input > > methods. I think these method names are more precise but the comments aren't > > wrong. Open to suggestions for clearer documentation. > > "Removes the input method from the list of enabled methods, disabling the input > method for the current user." ? Done.
lgtm |
