Implement "Add" and "Remove" buttons in Language and Input page.

Implement "Add" and "Remove" buttons in Language and Input page.

Along the way, remove translation for language codes in
|localStrings| (ex "fr" -> "French").
Instead introduce templateData.languageList, with which
we can retrieve language information.

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

[satorux1, 2010-08-02 10:30:14]

Erik,

I've just uploaded a new patch. gclient sync resulted in lots of conflicts, due
to:

http://src.chromium.org/viewvc/chrome?view=rev&revision=54350

I've also added some code and done some refactoring. Please take a look.

Thanks,
Satoru

[arv (Not doing code reviews), 2010-08-02 16:46:06]

http://codereview.chromium.org/3041025/diff/4001/5004
File chrome/browser/resources/options/chromeos_language_add_language_overlay.js
(right):

http://codereview.chromium.org/3041025/diff/4001/5004#newcode48
chrome/browser/resources/options/chromeos_language_add_language_overlay.js:48:
a.href = '#';  // Add href to make it look like a regular link.
Only use links if it makes sense to open the link in a new tab.

The standard solution is to use a <button> styled as a link.

http://codereview.chromium.org/3041025/diff/4001/5004#newcode55
chrome/browser/resources/options/chromeos_language_add_language_overlay.js:55:
li.languageCode = language.code;
Do you really need to add languageCode to both the a and li?

http://codereview.chromium.org/3041025/diff/4001/5004#newcode68
chrome/browser/resources/options/chromeos_language_add_language_overlay.js:68:
e.preventDefault();  // Cancel the link click.
If this is all we are doing in the onclick I would just add a single listener to
the addLanguageList element that did this.

http://codereview.chromium.org/3041025/diff/4001/5005
File chrome/browser/resources/options/chromeos_language_list.js (right):

http://codereview.chromium.org/3041025/diff/4001/5005#newcode25
chrome/browser/resources/options/chromeos_language_list.js:25: if
(!this.languageCodeToDisplayName) {
trailing underscore to mark this as private?

http://codereview.chromium.org/3041025/diff/4001/5005#newcode66
chrome/browser/resources/options/chromeos_language_list.js:66:
dataModel.splice(dataModel.length, 0, languageCode);
dataModel.push(languageCode) FTW

You could keep both the languageCode and the displayName in the data model. The
data model can contain objects and it is not limited to vaue types.

http://codereview.chromium.org/3041025/diff/4001/5005#newcode82
chrome/browser/resources/options/chromeos_language_list.js:82: return
this.dataModel.item(this.selectionModel.selectedIndex);
The list has a convenience function for this already

return this.selectedItem;

http://codereview.chromium.org/3041025/diff/4001/5005#newcode97
chrome/browser/resources/options/chromeos_language_list.js:97: // Select the
item at the original index if possible. Otherwise,
This part should be handled by the selection model as well

http://codereview.chromium.org/3041025/diff/4001/5006
File chrome/browser/resources/options/chromeos_language_options.css (right):

http://codereview.chromium.org/3041025/diff/4001/5006#newcode66
chrome/browser/resources/options/chromeos_language_options.css:66:
-webkit-column-count: 4;
Nice. Should we allow left and right keyboard shortcuts to change the selection
easier?

http://codereview.chromium.org/3041025/diff/4001/5007
File chrome/browser/resources/options/chromeos_language_options.js (right):

http://codereview.chromium.org/3041025/diff/4001/5007#newcode203
chrome/browser/resources/options/chromeos_language_options.js:203: var
engines_to_be_removed = {};
no underscores in js variable names

[satorux1, 2010-08-03 09:18:31]

Erik,

Thank you the feedback. I've addressed your comments. Please take another look.

http://codereview.chromium.org/3041025/diff/4001/5004
File chrome/browser/resources/options/chromeos_language_add_language_overlay.js
(right):

http://codereview.chromium.org/3041025/diff/4001/5004#newcode48
chrome/browser/resources/options/chromeos_language_add_language_overlay.js:48:
a.href = '#';  // Add href to make it look like a regular link.
On 2010/08/02 16:46:06, arv wrote:
> Only use links if it makes sense to open the link in a new tab.
> 
> The standard solution is to use a <button> styled as a link.

Done.

http://codereview.chromium.org/3041025/diff/4001/5004#newcode55
chrome/browser/resources/options/chromeos_language_add_language_overlay.js:55:
li.languageCode = language.code;
On 2010/08/02 16:46:06, arv wrote:
> Do you really need to add languageCode to both the a and li?

Good point. Removed langugeCode from li.

http://codereview.chromium.org/3041025/diff/4001/5004#newcode68
chrome/browser/resources/options/chromeos_language_add_language_overlay.js:68:
e.preventDefault();  // Cancel the link click.
On 2010/08/02 16:46:06, arv wrote:
> If this is all we are doing in the onclick I would just add a single listener
to
> the addLanguageList element that did this.

Good point. Removed.

http://codereview.chromium.org/3041025/diff/4001/5005
File chrome/browser/resources/options/chromeos_language_list.js (right):

http://codereview.chromium.org/3041025/diff/4001/5005#newcode25
chrome/browser/resources/options/chromeos_language_list.js:25: if
(!this.languageCodeToDisplayName) {
On 2010/08/02 16:46:06, arv wrote:
> trailing underscore to mark this as private?

Done.

http://codereview.chromium.org/3041025/diff/4001/5005#newcode66
chrome/browser/resources/options/chromeos_language_list.js:66:
dataModel.splice(dataModel.length, 0, languageCode);
On 2010/08/02 16:46:06, arv wrote:
> dataModel.push(languageCode) FTW

Done.
 
> You could keep both the languageCode and the displayName in the data model.
The
> data model can contain objects and it is not limited to vaue types.

Thank you for the input. I thought about it , but for now it looks ok to use
language codes as the data model.

http://codereview.chromium.org/3041025/diff/4001/5005#newcode82
chrome/browser/resources/options/chromeos_language_list.js:82: return
this.dataModel.item(this.selectionModel.selectedIndex);
On 2010/08/02 16:46:06, arv wrote:
> The list has a convenience function for this already
> 
> return this.selectedItem;

Done.

http://codereview.chromium.org/3041025/diff/4001/5005#newcode97
chrome/browser/resources/options/chromeos_language_list.js:97: // Select the
item at the original index if possible. Otherwise,
On 2010/08/02 16:46:06, arv wrote:
> This part should be handled by the selection model as well

Added a TODO comment.

http://codereview.chromium.org/3041025/diff/4001/5006
File chrome/browser/resources/options/chromeos_language_options.css (right):

http://codereview.chromium.org/3041025/diff/4001/5006#newcode66
chrome/browser/resources/options/chromeos_language_options.css:66:
-webkit-column-count: 4;
On 2010/08/02 16:46:06, arv wrote:
> Nice. Should we allow left and right keyboard shortcuts to change the
selection
> easier?

That would be cool, but would it be hard to implement?

BTW, changed the number of columns to 2 per suggestion from Cole.

http://codereview.chromium.org/3041025/diff/4001/5007
File chrome/browser/resources/options/chromeos_language_options.js (right):

http://codereview.chromium.org/3041025/diff/4001/5007#newcode203
chrome/browser/resources/options/chromeos_language_options.js:203: var
engines_to_be_removed = {};
On 2010/08/02 16:46:06, arv wrote:
> no underscores in js variable names

Done.

[arv (Not doing code reviews), 2010-08-03 17:11:45]

LGTM

http://codereview.chromium.org/3041025/diff/15001/16007
File chrome/browser/resources/options/chromeos_language_options.css (right):

http://codereview.chromium.org/3041025/diff/15001/16007#newcode74
chrome/browser/resources/options/chromeos_language_options.css:74: cursor:
pointer;
Do you also want color: blue?

I would actually prefer that we add a link-button class that can be reused by
people.

.link-button {
  background-color: transparent;
  border: none;
  color: blue;
  cursor: pointer;
  text-decoration: underline;
}

[satorux1, 2010-08-03 22:48:29]

http://codereview.chromium.org/3041025/diff/15001/16007
File chrome/browser/resources/options/chromeos_language_options.css (right):

http://codereview.chromium.org/3041025/diff/15001/16007#newcode74
chrome/browser/resources/options/chromeos_language_options.css:74: cursor:
pointer;
On 2010/08/03 17:11:45, arv wrote:
> Do you also want color: blue?

Good point. I'll add it.



> I would actually prefer that we add a link-button class that can be reused by
> people.
> 
> .link-button {

I assume you meant #link-button


>   background-color: transparent;
>   border: none;
>   color: blue;
>   cursor: pointer;
>   text-decoration: underline;
> }

Good idea. Where would be the right place for sharing CSS? Maybe
options_page.css?

[arv (Not doing code reviews), 2010-08-03 22:57:11]

http://codereview.chromium.org/3041025/diff/15001/16007
File chrome/browser/resources/options/chromeos_language_options.css (right):

http://codereview.chromium.org/3041025/diff/15001/16007#newcode74
chrome/browser/resources/options/chromeos_language_options.css:74: cursor:
pointer;
On 2010/08/03 22:48:29, satorux1 wrote:
> On 2010/08/03 17:11:45, arv wrote:
> > Do you also want color: blue?
> 
> Good point. I'll add it.
> 
> 
> 
> > I would actually prefer that we add a link-button class that can be reused
by
> > people.
> > 
> > .link-button {
> 
> I assume you meant #link-button

I meant .link-button. # is for id and ids are supposed to be unique in the
document. Using . allows all elements with the class "link-button" to look like
link buttons.

> 
> 
> >   background-color: transparent;
> >   border: none;
> >   color: blue;
> >   cursor: pointer;
> >   text-decoration: underline;
> > }
> 
> Good idea. Where would be the right place for sharing CSS? Maybe
> options_page.css?

options_page.css seems to contain top level css already so that seems good
enough.

[satorux1, 2010-08-03 23:04:36]

http://codereview.chromium.org/3041025/diff/15001/16007
File chrome/browser/resources/options/chromeos_language_options.css (right):

http://codereview.chromium.org/3041025/diff/15001/16007#newcode74
chrome/browser/resources/options/chromeos_language_options.css:74: cursor:
pointer;
On 2010/08/03 22:57:11, arv wrote:
> On 2010/08/03 22:48:29, satorux1 wrote:
> > On 2010/08/03 17:11:45, arv wrote:
> > > Do you also want color: blue?
> > 
> > Good point. I'll add it.
> > 
> > 
> > 
> > > I would actually prefer that we add a link-button class that can be reused
> by
> > > people.
> > > 
> > > .link-button {
> > 
> > I assume you meant #link-button
> 
> I meant .link-button. # is for id and ids are supposed to be unique in the
> document. Using . allows all elements with the class "link-button" to look
like
> link buttons.

My bad. I'm easily confused about # and .


> 
> > 
> > 
> > >   background-color: transparent;
> > >   border: none;
> > >   color: blue;
> > >   cursor: pointer;
> > >   text-decoration: underline;
> > > }
> > 
> > Good idea. Where would be the right place for sharing CSS? Maybe
> > options_page.css?
> 
> options_page.css seems to contain top level css already so that seems good
> enough.

Sounds good. I'll move it to options_page.css