Display the language name and the input method list dynamically.

Display the language name and the input method list dynamically.

Before this change, the language name and the input method list were
hard-coded in the HTML file. With this change, these values will be
displayed based on the underlying data from C++ world.

TEST=manually
BUG=chromium-os:4573
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=53737

[arv (Not doing code reviews), 2010-07-21 17:23:12]

http://codereview.chromium.org/3008016/diff/1/4
File chrome/browser/resources/options/chromeos_language_list.js (right):

http://codereview.chromium.org/3008016/diff/1/4#newcode55
chrome/browser/resources/options/chromeos_language_list.js:55: addObserver:
function(observer) {
List is an EventTarget so you can use addEventListener and dispatchEvent. There
is a helper function called cr.dispatchSimpleEvent that makes dispatching events
easier.

Another option is to have a property for selectedLanguage and use property
change events. The cr.js library has a macro to create a property that handles
property change events for you.

cr.defineProperty(LanguageList, 'selectedLanguage');

then any time you set selectedLanguage to a new value (not the same value as
defined by ===) the object dispatches a 'selectedLanguageChange' event.

The original goal of property change events was to build a data binding layer on
top of them but so far the UI has not really required any utility functions. So,
to use them you need to listen to the event.

myLanguageList.addEventListener('selectedLanguageChange', function(e) {
  ...
});

You can also dispatch the property change event manually, which might make sense
in this case since you might not want to expose a setter for the language.


LanguageList.prototype = {
  ...
  get selectedLanguage() {
     return this.dataModel.item(this.selectedIndex);
  },
  handleChange: function(e) {
    ...
    cr.dispatchPropertyChange(this, 'selectedLanguage');
  },
  ...

};

http://codereview.chromium.org/3008016/diff/1/4#newcode65
chrome/browser/resources/options/chromeos_language_list.js:65: handleClick_:
function(e) {
handleClick_ is never used.

http://codereview.chromium.org/3008016/diff/1/4#newcode66
chrome/browser/resources/options/chromeos_language_list.js:66: var element =
e.target;
This is dead code.

http://codereview.chromium.org/3008016/diff/1/4#newcode100
chrome/browser/resources/options/chromeos_language_list.js:100: if
(this.dataModel.length > 0) {
No {} for single line ifs

http://codereview.chromium.org/3008016/diff/1/4#newcode102
chrome/browser/resources/options/chromeos_language_list.js:102: }
Can you file a bug to implement a single item selection model that does not
allow no selection?

http://codereview.chromium.org/3008016/diff/1/6
File chrome/browser/resources/options/chromeos_language_options.js (right):

http://codereview.chromium.org/3008016/diff/1/6#newcode50
chrome/browser/resources/options/chromeos_language_options.js:50: // Add all
input methods, but make all of them invisible here. We'll
Are there a lot of these? Creating DOM nodes is quite expensive so it might be
worth only creating the visible ones.

http://codereview.chromium.org/3008016/diff/1/6#newcode53
chrome/browser/resources/options/chromeos_language_options.js:53: for (var i =
0; i < inputMethodListData.length; ++i) {
i++ is more commonly used in js

http://codereview.chromium.org/3008016/diff/1/6#newcode59
chrome/browser/resources/options/chromeos_language_options.js:59: var span = 
document.createElement('span');
You can skip this span. Just append a text node instead.

Minimizing the DOM is good for performance but it also makes things easier to
maintain.

http://codereview.chromium.org/3008016/diff/1/6#newcode69
chrome/browser/resources/options/chromeos_language_options.js:69:
this.inputMethodLabels_.push(label);
I would skip this array. You can get all the labels from the DOM using something
like

inputMethodList.querySelectorAll('label')

http://codereview.chromium.org/3008016/diff/1/6#newcode86
chrome/browser/resources/options/chromeos_language_options.js:86: * @param
{String} languageCode Language code (ex. "fr").
string (String is for the object wrapped string which should never be used)

http://codereview.chromium.org/3008016/diff/1/6#newcode89
chrome/browser/resources/options/chromeos_language_options.js:89:
onSelectedLanguageChanged: function(languageCode, languageDisplayName) {
name these handleEventName_ since onfoo is commonly used for assigning new
single event listeners.

[satorux1, 2010-07-22 08:48:42]

Thank you for the great review comments. It was again a good learning experience
for me.

http://codereview.chromium.org/3008016/diff/1/4
File chrome/browser/resources/options/chromeos_language_list.js (right):

http://codereview.chromium.org/3008016/diff/1/4#newcode55
chrome/browser/resources/options/chromeos_language_list.js:55: addObserver:
function(observer) {
On 2010/07/21 17:23:13, arv wrote:
> List is an EventTarget so you can use addEventListener and dispatchEvent.
There
> is a helper function called cr.dispatchSimpleEvent that makes dispatching
events
> easier.
> 
> Another option is to have a property for selectedLanguage and use property
> change events. The cr.js library has a macro to create a property that handles
> property change events for you.
> 
> cr.defineProperty(LanguageList, 'selectedLanguage');
> 
> then any time you set selectedLanguage to a new value (not the same value as
> defined by ===) the object dispatches a 'selectedLanguageChange' event.
> 
> The original goal of property change events was to build a data binding layer
on
> top of them but so far the UI has not really required any utility functions.
So,
> to use them you need to listen to the event.
> 
> myLanguageList.addEventListener('selectedLanguageChange', function(e) {
>   ...
> });

This is very cool. Changed the code accordingly. Now the code becomes much
simpler.

> 
> You can also dispatch the property change event manually, which might make
sense
> in this case since you might not want to expose a setter for the language.
> 
> 
> LanguageList.prototype = {
>   ...
>   get selectedLanguage() {
>      return this.dataModel.item(this.selectedIndex);
>   },
>   handleChange: function(e) {
>     ...
>     cr.dispatchPropertyChange(this, 'selectedLanguage');
>   },
>   ...
> 
> };

http://codereview.chromium.org/3008016/diff/1/4#newcode65
chrome/browser/resources/options/chromeos_language_list.js:65: handleClick_:
function(e) {
On 2010/07/21 17:23:13, arv wrote:
> handleClick_ is never used.

Done.

http://codereview.chromium.org/3008016/diff/1/4#newcode66
chrome/browser/resources/options/chromeos_language_list.js:66: var element =
e.target;
On 2010/07/21 17:23:13, arv wrote:
> This is dead code.

Done.

http://codereview.chromium.org/3008016/diff/1/4#newcode100
chrome/browser/resources/options/chromeos_language_list.js:100: if
(this.dataModel.length > 0) {
On 2010/07/21 17:23:13, arv wrote:
> No {} for single line ifs

Done.

http://codereview.chromium.org/3008016/diff/1/4#newcode102
chrome/browser/resources/options/chromeos_language_list.js:102: }
On 2010/07/21 17:23:13, arv wrote:
> Can you file a bug to implement a single item selection model that does not
> allow no selection?

Done.

http://code.google.com/p/chromium/issues/detail?id=49893

http://codereview.chromium.org/3008016/diff/1/6
File chrome/browser/resources/options/chromeos_language_options.js (right):

http://codereview.chromium.org/3008016/diff/1/6#newcode50
chrome/browser/resources/options/chromeos_language_options.js:50: // Add all
input methods, but make all of them invisible here. We'll
On 2010/07/21 17:23:13, arv wrote:
> Are there a lot of these? Creating DOM nodes is quite expensive so it might be
> worth only creating the visible ones.

Not that many. We have 70 now, and it won't grow much. Added a comment about it.

http://codereview.chromium.org/3008016/diff/1/6#newcode53
chrome/browser/resources/options/chromeos_language_options.js:53: for (var i =
0; i < inputMethodListData.length; ++i) {
On 2010/07/21 17:23:13, arv wrote:
> i++ is more commonly used in js

Done.

http://codereview.chromium.org/3008016/diff/1/6#newcode59
chrome/browser/resources/options/chromeos_language_options.js:59: var span = 
document.createElement('span');
On 2010/07/21 17:23:13, arv wrote:
> You can skip this span. Just append a text node instead.
> 
> Minimizing the DOM is good for performance but it also makes things easier to
> maintain.

Done.

http://codereview.chromium.org/3008016/diff/1/6#newcode69
chrome/browser/resources/options/chromeos_language_options.js:69:
this.inputMethodLabels_.push(label);
On 2010/07/21 17:23:13, arv wrote:
> I would skip this array. You can get all the labels from the DOM using
something
> like
> 
> inputMethodList.querySelectorAll('label')

Done.

http://codereview.chromium.org/3008016/diff/1/6#newcode86
chrome/browser/resources/options/chromeos_language_options.js:86: * @param
{String} languageCode Language code (ex. "fr").
On 2010/07/21 17:23:13, arv wrote:
> string (String is for the object wrapped string which should never be used)

Done.

http://codereview.chromium.org/3008016/diff/1/6#newcode89
chrome/browser/resources/options/chromeos_language_options.js:89:
onSelectedLanguageChanged: function(languageCode, languageDisplayName) {
On 2010/07/21 17:23:13, arv wrote:
> name these handleEventName_ since onfoo is commonly used for assigning new
> single event listeners.

Done.

[dhg, 2010-07-22 15:21:57]

http://codereview.chromium.org/3008016/diff/6001/7001
File chrome/browser/chromeos/dom_ui/language_options_handler.cc (right):

http://codereview.chromium.org/3008016/diff/6001/7001#newcode17
chrome/browser/chromeos/dom_ui/language_options_handler.cc:17: using namespace
chromeos;
From the style guide:
You may not use a using-directive to make all names from a namespace available.
// Forbidden -- This pollutes the namespace.
using namespace foo;
You may use a using-declaration anywhere in a .cc file, and in functions,
methods or classes in .h files.
// OK in .cc files.
// Must be in a function, method or class in .h files.
using ::foo::bar;

[satorux1, 2010-07-23 01:29:38]

http://codereview.chromium.org/3008016/diff/6001/7001
File chrome/browser/chromeos/dom_ui/language_options_handler.cc (right):

http://codereview.chromium.org/3008016/diff/6001/7001#newcode17
chrome/browser/chromeos/dom_ui/language_options_handler.cc:17: using namespace
chromeos;
Good catch! Fixed.

[dhg, 2010-07-23 15:52:07]

the C++ looks fine, but I'm not sure how arv feels about the adding to cr.
here... so i'll leave the rest to him.

http://codereview.chromium.org/3008016/diff/12001/13003
File chrome/browser/resources/options.html (right):

http://codereview.chromium.org/3008016/diff/12001/13003#newcode25
chrome/browser/resources/options.html:25: <script
src="options/chromeos_language_options.js"></script>
Alpha  (looks like the one after this is out of order)

[arv (Not doing code reviews), 2010-07-23 17:40:44]

http://codereview.chromium.org/3008016/diff/12001/13004
File chrome/browser/resources/options/chromeos_language_list.js (right):

http://codereview.chromium.org/3008016/diff/12001/13004#newcode56
chrome/browser/resources/options/chromeos_language_list.js:56:
this.selectedLanguageIndex = this.selectionModel.selectedIndex;
This does not seem to provide any significant info after all. The List has a
selectedItem property already and it seems fine to listen to the 'change' event
for notifications.

http://codereview.chromium.org/3008016/diff/12001/13006
File chrome/browser/resources/options/chromeos_language_options.js (right):

http://codereview.chromium.org/3008016/diff/12001/13006#newcode33
chrome/browser/resources/options/chromeos_language_options.js:33:
languageOptionsList.addEventListener('selectedLanguageIndexChange',
Yeah, it seems like 'change' should be sufficient after all. Sorry for leading
you astray.

[satorux1, 2010-07-26 05:03:38]

Thank you for the code review. Addressed the comments. Please take another look.

http://codereview.chromium.org/3008016/diff/12001/13003
File chrome/browser/resources/options.html (right):

http://codereview.chromium.org/3008016/diff/12001/13003#newcode25
chrome/browser/resources/options.html:25: <script
src="options/chromeos_language_options.js"></script>
On 2010/07/23 15:52:07, dhg wrote:
> Alpha  (looks like the one after this is out of order)

Done. chromeos_language_hangul_options.js is now removed. :)

http://codereview.chromium.org/3008016/diff/12001/13004
File chrome/browser/resources/options/chromeos_language_list.js (right):

http://codereview.chromium.org/3008016/diff/12001/13004#newcode56
chrome/browser/resources/options/chromeos_language_list.js:56:
this.selectedLanguageIndex = this.selectionModel.selectedIndex;
On 2010/07/23 17:40:44, arv wrote:
> This does not seem to provide any significant info after all. The List has a
> selectedItem property already and it seems fine to listen to the 'change'
event
> for notifications.

Good point. Changed accordingly.

http://codereview.chromium.org/3008016/diff/12001/13006
File chrome/browser/resources/options/chromeos_language_options.js (right):

http://codereview.chromium.org/3008016/diff/12001/13006#newcode33
chrome/browser/resources/options/chromeos_language_options.js:33:
languageOptionsList.addEventListener('selectedLanguageIndexChange',
On 2010/07/23 17:40:44, arv wrote:
> Yeah, it seems like 'change' should be sufficient after all. Sorry for leading
> you astray.

Fixed. Never mind. It was a good technique to know. :)

[arv (Not doing code reviews), 2010-07-26 22:24:26]

lgtm