Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12087105:
Use drop-down UI for "add language". (Closed)
|
Created:
7 years, 10 months ago by Seigo Nonaka Modified:
7 years, 10 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, arv (Not doing code reviews), yusukes+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUse drop-down UI for "add language".
This patch changes "add language" overlay page in chrome://settings/language
from listed links to drop-down menu.
This patch only affects Chrome OS settings page.
BUG=133346
TEST=try bots
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182203
Patch Set 1 #
Total comments: 10
Patch Set 2 : Address comments #Patch Set 3 : rebasing #
Messages
Total messages: 13 (0 generated)
Hi zork, could you check my patch before sending owner review, because this is my first time to modify html/js code in Chrome. Thanks
lgtm
zork: Thank you for your review. Adding dbeam@ as the owner of option page. Hi Dan, Could you take a look? This patch changes "Add language" overlay in Chrome OS. Thank you.
screenshots and/or mocks?
https://codereview.chromium.org/12087105/diff/1/chrome/browser/resources/opti... File chrome/browser/resources/options/language_add_language_overlay.js (right): https://codereview.chromium.org/12087105/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/language_add_language_overlay.js:47: if (language.displayName != language.nativeDisplayName) { optional nit: no curlies {} https://codereview.chromium.org/12087105/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/language_add_language_overlay.js:51: var option = document.createElement('option'); nit: cr.doc.createElement() or this.ownerDocument.createElement() IMO https://codereview.chromium.org/12087105/diff/1/chrome/browser/resources/opti... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/12087105/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/language_options.js:133: so this is available on CrOS now? seems like it wasn't before... https://codereview.chromium.org/12087105/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/language_options.js:135: var addLanguageOkButton = $('add-language-overlay-ok-button'); nit: inline `addLanguageOkButton` IMO https://codereview.chromium.org/12087105/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/language_options.js:138: this.handleAddLanguageOkButtonClick_.bind(this)); nit: 'click', this.handleAddLanguageOkButtonClick_.bind(this)); if it fits
Uploaded screenshot to https://code.google.com/p/chromium/issues/detail?id=133346#c38 PTAL? Thanks https://codereview.chromium.org/12087105/diff/1/chrome/browser/resources/opti... File chrome/browser/resources/options/language_add_language_overlay.js (right): https://codereview.chromium.org/12087105/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/language_add_language_overlay.js:47: if (language.displayName != language.nativeDisplayName) { On 2013/02/01 19:38:10, Dan Beam wrote: > optional nit: no curlies {} Done. https://codereview.chromium.org/12087105/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/language_add_language_overlay.js:51: var option = document.createElement('option'); On 2013/02/01 19:38:10, Dan Beam wrote: > nit: cr.doc.createElement() or this.ownerDocument.createElement() IMO Done. https://codereview.chromium.org/12087105/diff/1/chrome/browser/resources/opti... File chrome/browser/resources/options/language_options.js (right): https://codereview.chromium.org/12087105/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/language_options.js:133: Yes. Before UI didn't have OK button because all items were listed and clickable. After this patch, all items are in "select" tag and user can proceed with "OK" button. On 2013/02/01 19:38:10, Dan Beam wrote: > so this is available on CrOS now? seems like it wasn't before... https://codereview.chromium.org/12087105/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/language_options.js:135: var addLanguageOkButton = $('add-language-overlay-ok-button'); On 2013/02/01 19:38:11, Dan Beam wrote: > nit: inline `addLanguageOkButton` IMO Done. https://codereview.chromium.org/12087105/diff/1/chrome/browser/resources/opti... chrome/browser/resources/options/language_options.js:138: this.handleAddLanguageOkButtonClick_.bind(this)); On 2013/02/01 19:38:11, Dan Beam wrote: > nit: 'click', this.handleAddLanguageOkButtonClick_.bind(this)); > > if it fits Done.
lgtm
Thank you for your review! submitting...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nona@chromium.org/12087105/7001
Failed to apply patch for chrome/browser/resources/options/language_options.js:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/browser/resources/options/language_options.js
Hunk #1 FAILED at 125.
Hunk #2 succeeded at 676 (offset 14 lines).
Hunk #3 succeeded at 714 (offset 14 lines).
1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/resources/options/language_options.js.rej
Patch: chrome/browser/resources/options/language_options.js
Index: chrome/browser/resources/options/language_options.js
diff --git a/chrome/browser/resources/options/language_options.js
b/chrome/browser/resources/options/language_options.js
index
dc58e76d5b350c7933a76aa7a8a2c92aed13d052..cd8e1c1ddf0c289bc2cb89e4e2ff209452bc6a3d
100644
--- a/chrome/browser/resources/options/language_options.js
+++ b/chrome/browser/resources/options/language_options.js
@@ -125,21 +125,17 @@ cr.define('options', function() {
}
if (cr.isChromeOS) {
- // Listen to user clicks on the add language list.
- var addLanguageList = $('add-language-overlay-language-list');
- addLanguageList.addEventListener(
- 'click',
- this.handleAddLanguageListClick_.bind(this));
+ // Listen to user click on the extension ime button.
$('language-options-extension-ime-button').addEventListener(
'click',
this.handleExtensionImeButtonClick_.bind(this));
- } else {
- // Listen to add language dialog ok button.
- var addLanguageOkButton = $('add-language-overlay-ok-button');
- addLanguageOkButton.addEventListener(
- 'click',
- this.handleAddLanguageOkButtonClick_.bind(this));
+ }
+
+ // Listen to add language dialog ok button.
+ $('add-language-overlay-ok-button').addEventListener(
+ 'click', this.handleAddLanguageOkButtonClick_.bind(this));
+ if (!cr.isChromeOS) {
// Show experimental features if enabled.
if (loadTimeData.getBoolean('enableSpellingAutoCorrect'))
$('auto-spell-correction-option').hidden = false;
@@ -662,34 +658,6 @@ cr.define('options', function() {
},
/**
- * Handles add language list's click event.
- * @param {Event} e Click event.
- */
- handleAddLanguageListClick_: function(e) {
- var languageOptionsList = $('language-options-list');
- var languageCode = e.target.languageCode;
- // languageCode can be undefined, if click was made on some random
- // place in the overlay, rather than a button. Ignore it.
- if (!languageCode) {
- return;
- }
- languageOptionsList.addLanguage(languageCode);
- var inputMethodIds = this.languageCodeToInputMethodIdsMap_[languageCode];
- // Enable the first input method for the language added.
- if (inputMethodIds && inputMethodIds[0] &&
- // Don't add the input method it's already present. This can
- // happen if the same input method is shared among multiple
- // languages (ex. English US keyboard is used for English US and
- // Filipino).
- this.preloadEngines_.indexOf(inputMethodIds[0]) == -1) {
- this.preloadEngines_.push(inputMethodIds[0]);
- this.updateCheckboxesFromPreloadEngines_();
- this.savePreloadEnginesPref_();
- }
- OptionsPage.closeOverlay();
- },
-
- /**
* Handles extension IME button.
*/
handleExtensionImeButtonClick_: function() {
@@ -728,6 +696,21 @@ cr.define('options', function() {
if (selectedIndex >= 0) {
var selection = languagesSelect.options[selectedIndex];
$('language-options-list').addLanguage(String(selection.value));
+ if (cr.isChromeOS) {
+ var inputMethodIds =
+ this.languageCodeToInputMethodIdsMap_[selection.value];
+ // Enable the first input method for the language added.
+ if (inputMethodIds && inputMethodIds[0] &&
+ // Don't add the input method it's already present. This can
+ // happen if the same input method is shared among multiple
+ // languages (ex. English US keyboard is used for English US and
+ // Filipino).
+ this.preloadEngines_.indexOf(inputMethodIds[0]) == -1) {
+ this.preloadEngines_.push(inputMethodIds[0]);
+ this.updateCheckboxesFromPreloadEngines_();
+ this.savePreloadEnginesPref_();
+ }
+ }
OptionsPage.closeOverlay();
}
},
As talked with kenji@ who is our PM.
We decide to move target milestone of this patch to M27.
So let me keep this patch until M26 branch cut.
Thank you.
On 2013/02/05 04:29:53, I haz the power (commit-bot) wrote:
> Failed to apply patch for
chrome/browser/resources/options/language_options.js:
> While running patch -p1 --forward --force --no-backup-if-mismatch;
> patching file chrome/browser/resources/options/language_options.js
> Hunk #1 FAILED at 125.
> Hunk #2 succeeded at 676 (offset 14 lines).
> Hunk #3 succeeded at 714 (offset 14 lines).
> 1 out of 3 hunks FAILED -- saving rejects to file
> chrome/browser/resources/options/language_options.js.rej
>
> Patch: chrome/browser/resources/options/language_options.js
> Index: chrome/browser/resources/options/language_options.js
> diff --git a/chrome/browser/resources/options/language_options.js
> b/chrome/browser/resources/options/language_options.js
> index
>
dc58e76d5b350c7933a76aa7a8a2c92aed13d052..cd8e1c1ddf0c289bc2cb89e4e2ff209452bc6a3d
> 100644
> --- a/chrome/browser/resources/options/language_options.js
> +++ b/chrome/browser/resources/options/language_options.js
> @@ -125,21 +125,17 @@ cr.define('options', function() {
> }
>
> if (cr.isChromeOS) {
> - // Listen to user clicks on the add language list.
> - var addLanguageList = $('add-language-overlay-language-list');
> - addLanguageList.addEventListener(
> - 'click',
> - this.handleAddLanguageListClick_.bind(this));
> + // Listen to user click on the extension ime button.
> $('language-options-extension-ime-button').addEventListener(
> 'click',
> this.handleExtensionImeButtonClick_.bind(this));
> - } else {
> - // Listen to add language dialog ok button.
> - var addLanguageOkButton = $('add-language-overlay-ok-button');
> - addLanguageOkButton.addEventListener(
> - 'click',
> - this.handleAddLanguageOkButtonClick_.bind(this));
> + }
> +
> + // Listen to add language dialog ok button.
> + $('add-language-overlay-ok-button').addEventListener(
> + 'click', this.handleAddLanguageOkButtonClick_.bind(this));
>
> + if (!cr.isChromeOS) {
> // Show experimental features if enabled.
> if (loadTimeData.getBoolean('enableSpellingAutoCorrect'))
> $('auto-spell-correction-option').hidden = false;
> @@ -662,34 +658,6 @@ cr.define('options', function() {
> },
>
> /**
> - * Handles add language list's click event.
> - * @param {Event} e Click event.
> - */
> - handleAddLanguageListClick_: function(e) {
> - var languageOptionsList = $('language-options-list');
> - var languageCode = e.target.languageCode;
> - // languageCode can be undefined, if click was made on some random
> - // place in the overlay, rather than a button. Ignore it.
> - if (!languageCode) {
> - return;
> - }
> - languageOptionsList.addLanguage(languageCode);
> - var inputMethodIds =
this.languageCodeToInputMethodIdsMap_[languageCode];
> - // Enable the first input method for the language added.
> - if (inputMethodIds && inputMethodIds[0] &&
> - // Don't add the input method it's already present. This can
> - // happen if the same input method is shared among multiple
> - // languages (ex. English US keyboard is used for English US and
> - // Filipino).
> - this.preloadEngines_.indexOf(inputMethodIds[0]) == -1) {
> - this.preloadEngines_.push(inputMethodIds[0]);
> - this.updateCheckboxesFromPreloadEngines_();
> - this.savePreloadEnginesPref_();
> - }
> - OptionsPage.closeOverlay();
> - },
> -
> - /**
> * Handles extension IME button.
> */
> handleExtensionImeButtonClick_: function() {
> @@ -728,6 +696,21 @@ cr.define('options', function() {
> if (selectedIndex >= 0) {
> var selection = languagesSelect.options[selectedIndex];
> $('language-options-list').addLanguage(String(selection.value));
> + if (cr.isChromeOS) {
> + var inputMethodIds =
> + this.languageCodeToInputMethodIdsMap_[selection.value];
> + // Enable the first input method for the language added.
> + if (inputMethodIds && inputMethodIds[0] &&
> + // Don't add the input method it's already present. This can
> + // happen if the same input method is shared among multiple
> + // languages (ex. English US keyboard is used for English US
and
> + // Filipino).
> + this.preloadEngines_.indexOf(inputMethodIds[0]) == -1) {
> + this.preloadEngines_.push(inputMethodIds[0]);
> + this.updateCheckboxesFromPreloadEngines_();
> + this.savePreloadEnginesPref_();
> + }
> + }
> OptionsPage.closeOverlay();
> }
> },
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nona@chromium.org/12087105/11002
Message was sent while issue was closed.
Change committed as 182203 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
